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

[edk2-devel] [PATCH 0/4] 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.

Gerd Hoffmann (4):
  OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads
  OvmfPkg/VirtNorFlashDxe: clarify block write logic
  OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
  OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too

 OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 33 +++++++++++------------
 OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |  5 ++++
 2 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.43.0



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

[edk2-devel] [PATCH 1/4] OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads

[Gerd Hoffmann]

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

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 1afd60ce66eb..cdc809d75e3d 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -566,7 +566,7 @@ NorFlashWriteSingleBlock (
                Instance,
                Lba,
                Offset & ~BOUNDARY_OF_32_WORDS,
-               (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
+               (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1,
                Instance->ShadowBuffer
                );
     if (EFI_ERROR (Status)) {
-- 
2.43.0



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

Re: [edk2-devel] [PATCH 1/4] OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads

[Ard Biesheuvel]

On Fri, 12 Jan 2024 at 12:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> 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>

Thanks a lot for taking the time to track this down and fix it.

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 1afd60ce66eb..cdc809d75e3d 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -566,7 +566,7 @@ NorFlashWriteSingleBlock (
>                 Instance,
>                 Lba,
>                 Offset & ~BOUNDARY_OF_32_WORDS,
> -               (*NumBytes | BOUNDARY_OF_32_WORDS) + 1,
> +               (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1,
>                 Instance->ShadowBuffer
>                 );
>      if (EFI_ERROR (Status)) {
> --
> 2.43.0
>


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

[edk2-devel] [PATCH 2/4] OvmfPkg/VirtNorFlashDxe: clarify block write logic

[Gerd Hoffmann]

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

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index cdc809d75e3d..90db12716a4c 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -560,13 +560,18 @@ NorFlashWriteSingleBlock (
     // 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.
+    UINTN   Start, End;
+    UINT32  Index, Count;
+
+    Start = Offset & ~BOUNDARY_OF_32_WORDS;
+    End   = (Offset + *NumBytes + BOUNDARY_OF_32_WORDS) & ~BOUNDARY_OF_32_WORDS;
 
     // Read the old version of the data into the shadow buffer
     Status = NorFlashRead (
                Instance,
                Lba,
-               Offset & ~BOUNDARY_OF_32_WORDS,
-               (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1,
+               Start,
+               End - Start,
                Instance->ShadowBuffer
                );
     if (EFI_ERROR (Status)) {
@@ -599,25 +604,19 @@ NorFlashWriteSingleBlock (
       goto Exit;
     }
 
-    Status = NorFlashWriteBuffer (
-               Instance,
-               BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
-               P30_MAX_BUFFER_SIZE_IN_BYTES,
-               Instance->ShadowBuffer
-               );
-    if (EFI_ERROR (Status)) {
-      goto Exit;
-    }
-
-    if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
-      BlockAddress += P30_MAX_BUFFER_SIZE_IN_BYTES;
-
+    for (Index = 0, Count = (End - Start) / P30_MAX_BUFFER_SIZE_IN_BYTES;
+         Index < Count;
+         Index++, BlockAddress += P30_MAX_BUFFER_SIZE_IN_BYTES)
+    {
       Status = NorFlashWriteBuffer (
                  Instance,
                  BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
                  P30_MAX_BUFFER_SIZE_IN_BYTES,
-                 Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES
+                 Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES * Index
                  );
+      if (EFI_ERROR (Status)) {
+        goto Exit;
+      }
     }
 
 Exit:
-- 
2.43.0



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

Re: [edk2-devel] [PATCH 2/4] OvmfPkg/VirtNorFlashDxe: clarify block write logic

[Ard Biesheuvel]

On Fri, 12 Jan 2024 at 12:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> 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>

Reviewed-by: Ard Biesheuvel <ardb@kernel.org>

Nit below

> ---
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c | 31 +++++++++++++-------------
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index cdc809d75e3d..90db12716a4c 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -560,13 +560,18 @@ NorFlashWriteSingleBlock (
>      // 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.
> +    UINTN   Start, End;
> +    UINT32  Index, Count;
> +

These var declarations should be at the start of the function.

> +    Start = Offset & ~BOUNDARY_OF_32_WORDS;
> +    End   = (Offset + *NumBytes + BOUNDARY_OF_32_WORDS) & ~BOUNDARY_OF_32_WORDS;
>
>      // Read the old version of the data into the shadow buffer
>      Status = NorFlashRead (
>                 Instance,
>                 Lba,
> -               Offset & ~BOUNDARY_OF_32_WORDS,
> -               (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1,
> +               Start,
> +               End - Start,
>                 Instance->ShadowBuffer
>                 );
>      if (EFI_ERROR (Status)) {
> @@ -599,25 +604,19 @@ NorFlashWriteSingleBlock (
>        goto Exit;
>      }
>
> -    Status = NorFlashWriteBuffer (
> -               Instance,
> -               BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
> -               P30_MAX_BUFFER_SIZE_IN_BYTES,
> -               Instance->ShadowBuffer
> -               );
> -    if (EFI_ERROR (Status)) {
> -      goto Exit;
> -    }
> -
> -    if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) > P30_MAX_BUFFER_SIZE_IN_BYTES) {
> -      BlockAddress += P30_MAX_BUFFER_SIZE_IN_BYTES;
> -
> +    for (Index = 0, Count = (End - Start) / P30_MAX_BUFFER_SIZE_IN_BYTES;
> +         Index < Count;
> +         Index++, BlockAddress += P30_MAX_BUFFER_SIZE_IN_BYTES)
> +    {
>        Status = NorFlashWriteBuffer (
>                   Instance,
>                   BlockAddress + (Offset & ~BOUNDARY_OF_32_WORDS),
>                   P30_MAX_BUFFER_SIZE_IN_BYTES,
> -                 Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES
> +                 Instance->ShadowBuffer + P30_MAX_BUFFER_SIZE_IN_BYTES * Index
>                   );
> +      if (EFI_ERROR (Status)) {
> +        goto Exit;
> +      }
>      }
>
>  Exit:
> --
> 2.43.0
>


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

[edk2-devel] [PATCH 3/4] 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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
index 90db12716a4c..c631ffd3a62d 100644
--- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
+++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
@@ -555,7 +555,7 @@ 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)) {
+  if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (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 (#113718): https://edk2.groups.io/g/devel/message/113718
Mute This Topic: https://groups.io/mt/103680934/7686176
Group Owner: devel+owner@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [rebecca@openfw.io]
-=-=-=-=-=-=-=-=-=-=-=-

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

[Ard Biesheuvel]

On Fri, 12 Jan 2024 at 12:38, Gerd Hoffmann <kraxel@redhat.com> 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 | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> index 90db12716a4c..c631ffd3a62d 100644
> --- a/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> +++ b/OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c
> @@ -555,7 +555,7 @@ 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.

Please update the comment

> -  if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
> +  if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (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: Ard Biesheuvel <ardb@kernel.org>


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

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

Re: [edk2-devel] [PATCH 4/4] OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too

[Ard Biesheuvel]

On Fri, 12 Jan 2024 at 12:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> 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.
>

Why?

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

Re: [edk2-devel] [PATCH 4/4] OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too

[Gerd Hoffmann]

On Fri, Jan 12, 2024 at 01:16:47PM +0100, Ard Biesheuvel wrote:
> On Fri, 12 Jan 2024 at 12:38, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > 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.
> >
> 
> Why?

This can happen only at the end of the variable list, when the variable
driver tried to append a new entry but was interrupted early.

Also without a valid header you don't have NameSize and DataSize.  So
you can't calculate the size of the entry to continue following the
chain.

take care,
  Gerd



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

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

[Laszlo Ersek]

On 1/12/24 12:37, 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.
>
> Gerd Hoffmann (4):
>   OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads
>   OvmfPkg/VirtNorFlashDxe: clarify block write logic
>   OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
>   OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
>
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 33 +++++++++++------------
>  OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |  5 ++++
>  2 files changed, 21 insertions(+), 17 deletions(-)
>

Looking at the original code makes me throw a fit (no offense -- I don't
know who wrote it, and I don't want to check).

There is not a single diagram in the code, when that would be central to
the whole thing.


    0               128              256
    [----------------|----------------]
    ^         ^             ^
    |         |             |
    |         |     (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 & 0x7F; i.e., Offset within the "word";
    |         this is where the (logical) update is supposed to start
    |
    Offset & ~(UINTN)0x7F; i.e., Offset truncated to "word" boundary

In this diagram, NumBytes is already limited to 256; that's because of
the existent condition

   if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {

So, independently of the bug in the code that this series is supposed to
fix, some problems with the original code:

- no diagram (see above)

- rampant duplication of hard to understand expressions, such as:

  - Offset & ~BOUNDARY_OF_32_WORDS

    (side comment: applying the bit-neg on a *signed integer* deserves
    its own brown paper bag)

  - *NumBytes + (Offset & BOUNDARY_OF_32_WORDS)

  - Offset & ~BOUNDARY_OF_32_WORDS

- more bit-neg applied to a *signed integer*:

  ~OrigData[CurOffset]

    because OrigData[CurOffset] is a UINT8, which gets promoted to
    INT32, and that's when the bit-neg is applied

- when the second word write is deemed necessary, then the
  *BlockAddress* variable is bumped by 128 bytes out of laziness for
  said second write -- and that is a *semantic wreck*. The BlockAddress
  does not change *at all*; it's the start offset within the block that
  increases by 128 bytes for the second word write.

- The weird Exit and DoErase labels are fugly. The function should
  either be split into two functions, or at least reorganized with "ifs"
  such that this jumping is not necessary. Gotos are fine, but only for
  error paths / cleanup on exit, not for business logic selection. IOW,
  the main offender is DoErase.


Then comments on the patch set:

- In my opinion, the series should progress in opposite order. First
  introduce a diagram (!), then refactor with the helper variables, and
  then fix the bug. With the refactoring in place *first*, the bugfix
  should be easier to understand. Then, potentially, generalize the code
  to larger-than-two multiples of a word, for writes.

- The first patch in the series is wrong.

  In case we need not erase the whole flash block, we will want to write
  one or two (consecutive) 128-byte "words". That is, 128 bytes, or 256
  bytes. That means we need to read the exact same byte counts as well.

  The *second* patch in the series actually seems to do this, with

    End   = (Offset + *NumBytes + BOUNDARY_OF_32_WORDS) & ~BOUNDARY_OF_32_WORDS;

  (This *in itself* would *much better* be written as follows:

    End = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);

  but I digress.)

  However, the first patch still introduces:

    (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1

  as the byte count for the read.

  Unfortunately, the "saturation logic" (i.e., OR-ing 0x7F to the
  exclusive end offset, for "seeking" to the end of the word), and then
  adding 1, does not implement a correct "align-up" operation.

  Consider

    Offset == 0 && *NumBytes == 256

  This circumstance is *valid* for the optimization path (and it is
  correctly permitted by the top-most check).

  But the expression introduced by patch#1 produces *384* for it, which
  is wrong.

  Similarly, given (for example)

    Offset == 1 && *NumBytes == 127

  the formula from patch#1 evaluates to 256.

  The expression does not consider the case when the exclusive end
  offset of the requested (logical write) is immediately at a word
  boundary (i.e., a multiple of 128). In that case namely, saturating
  with the bit-or, and adding 1, is wrong -- because in that case, no
  additional block should be read at all.

  So the first patch in the series replaces the *pre-series* bug with a
  different (less harmful) bug, and then the second patch silently
  *fixes* the replacement bug.

- This is in fact the fundamental bug: the incorrect implementation of
  the "align-up" operation with "saturate, then add 1". Both the
  pre-series code, and the code in patch#1, contain this mistake.

  The only thing that patch#1 changes is the *input*, to which the
  (incorrect) operation is applied -- namely in patch#1, the *input*
  changes from "NumBytes" to "exclusive end offset of the logical write,
  relative to the start of the (double-)word".

  That input change is in fact good (and *necessary*), but it's not
  *sufficient*. The operation itself needs to be fixed.

Summary:

- please rewrite the series in the following order: refactoring, then
  bugfix, then further armoring (additional sanity checks).

- please only ever apply the bit-neg operator on values that are UINT32,
  UINTN, or UINT64. Otherwise we get sign bit flipping, and that's
  terrible. (Most people are not even aware of it happening.)

- bit-fiddling should be kept to the absolute minimum. This means both a
  need for helper variables (calculated as early as possible), and usage
  of macros such as ALIGN_VALUE rather-than open-coded logic.

It's possible that the refactoring in patch#2 is effectively impossible
to do without fixing the *pre-series* bug at once. That's fine, as long
as we point out the bug in the commit message.

Importantly, the commit message should provide an actual (Offset,
*NumBytes) tuple (an example) where the pre-series expression

  (*NumBytes | BOUNDARY_OF_32_WORDS) + 1

calculates a bogus byte count for the read.

IOW, there are two things to highlight in the commit message:

- round-up operation incorrectly implemented,

- wrong input provided to the (already incorrect) round-up operation.

Thanks
Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113814): https://edk2.groups.io/g/devel/message/113814
Mute This Topic: https://groups.io/mt/103680930/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 0/4] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements

[Laszlo Ersek]

On 1/15/24 11:21, Laszlo Ersek wrote:

> - please only ever apply the bit-neg operator on values that are UINT32,
>   UINTN, or UINT64. Otherwise we get sign bit flipping, and that's
>   terrible. (Most people are not even aware of it happening.)

Doing this is BTW not as obvious as it might seem, at first sight. It's
good to remember some points about integer constants:

- assuming a naked constant (no 0x or 0 prefix, and no suffix such as l,
or u), the types considered are int, long, long long, in this order, by
the compiler, for the value (whichever fits first). That is: a "naked"
integer constant will *always* be signed.

- assuming an octal or hex prefix (0 or 0x), the candidate type list is
only *extended*; in other words, these prefixes don't *force* the type
to be unsigned, only *permit* it. The list becomes int, unsigned, long,
unsigned long, long long, unsigned long long. This is why 0x7F is just
"int", for example. However, 0x8000_0000 is not "int" anymore, but
"unsigned" (the value doesn't fit in "int", and the 0x prefix "permits"
"unsigned int").

- The suffixes do restrict the candidate type list. The "u" (and U)
suffixes remove the signed types, and add in the unsigned types. The
list becomes unsigned, unsigned long, unsigned long long. Furthermore,
the "l" and "ll" suffixes force (restrict) the type selection along a
different axis: they set the minimum integer "conversion rank", so to
say. The head of the list is trimmed so that the first candidate have
the specified rank. So with just an "l" suffix, the normal candidate
type list "int, long, long long" gets trimmed to "long, long long".
Assuming an "u" suffix in place already, adding the "l" suffix trims the
candidate type list "unsigned, unsigned long, unsigned long long" to
"unsigned long, unsigned long long". Assuming a 0x prefix and no "u"
suffix to begin with, appending the "l" suffix trims the type list "int,
unsigned, long, unsigned long, long long, unsigned long long" to "long,
unsigned long, long long, unsigned long long".

The "shorthand" to remember is: "prefixes permit, suffixes force".

Why I'm posting this wall of text: if we have a macro
BOUNDARY_OF_32_WORDS #defined as 0x7F, or a macro BIT1 #defined as
0x00000002, those are *signed int* values. And applying the bit-neg
operator ~ to them directly will flip the sign bit, and the resultant
value will be *implementation-dependent*. Given that we use two's
complement representation, the resultant value will always be signed int
with a negative value. (In a sign-and-magnitude representation e.g.,
where there is +0 and -0, we'd have to think further.) And then, for
example in:

  Offset & ~BOUNDARY_OF_32_WORDS

the negative value of the RHS is converted to the (unsigned) type of the
LHS [*], due to the default arithmetic conversions that are specified
for the & operator (too). This is done with the usual modular addition /
reduction.

So, when most people think that the above expression is simple
bit-fiddling, there are actually *two steps* that they miss: first, the
creation of a negative value of type "signed int" (using two's
complement representation), and then the reduction of that negative
"signed int" value to the (possibly wider) unsigned value range that the
other type is capable of representing [*].

[*] I'm taking some shortcuts here. The actual result type of the usual
arithmetic conversions (the "common real type for the operands
and result") is more complicated, but I won't describe all that here. It
can be read in the C std (drafts).

This is why I insist on one of two things in all such cases:

- either writing the expression as

    Offset & ~(UINTN)BOUNDARY_OF_32_WORDS

  where UINTN is supposed to match the type of Offset precisely,

- or #defining BOUNDARY_OF_32_WORDS already as an unsigned type --
either with an explicit cast ((UINTN)0x7F), or with a suitable suffix
(0x7Fllu).

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113820): https://edk2.groups.io/g/devel/message/113820
Mute This Topic: https://groups.io/mt/103680930/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 0/4] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements

[Ard Biesheuvel]

On Mon, 15 Jan 2024 at 11:21, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/12/24 12:37, 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.
> >
> > Gerd Hoffmann (4):
> >   OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads
> >   OvmfPkg/VirtNorFlashDxe: clarify block write logic
> >   OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
> >   OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
> >
> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 33 +++++++++++------------
> >  OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |  5 ++++
> >  2 files changed, 21 insertions(+), 17 deletions(-)
> >
>
> Looking at the original code makes me throw a fit (no offense -- I don't
> know who wrote it, and I don't want to check).
>

Hi Laszlo,

I am not the author of the original code, but I suppose I should take
at least some of the blame here, having added some of the logic to
reduce the number of MMIO accesses (which are disproportionately
expensive under virtualization), and this is where the bug got
introduced afaict.

> There is not a single diagram in the code, when that would be central to
> the whole thing.
>
>
>     0               128              256
>     [----------------|----------------]
>     ^         ^             ^
>     |         |             |
>     |         |     (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 & 0x7F; i.e., Offset within the "word";
>     |         this is where the (logical) update is supposed to start
>     |
>     Offset & ~(UINTN)0x7F; i.e., Offset truncated to "word" boundary
>
> In this diagram, NumBytes is already limited to 256; that's because of
> the existent condition
>
>    if ((*NumBytes + (Offset & BOUNDARY_OF_32_WORDS)) <= (2 * P30_MAX_BUFFER_SIZE_IN_BYTES)) {
>
> So, independently of the bug in the code that this series is supposed to
> fix, some problems with the original code:
>
> - no diagram (see above)
>
> - rampant duplication of hard to understand expressions, such as:
>
>   - Offset & ~BOUNDARY_OF_32_WORDS
>
>     (side comment: applying the bit-neg on a *signed integer* deserves
>     its own brown paper bag)
>
>   - *NumBytes + (Offset & BOUNDARY_OF_32_WORDS)
>
>   - Offset & ~BOUNDARY_OF_32_WORDS
>
> - more bit-neg applied to a *signed integer*:
>
>   ~OrigData[CurOffset]
>
>     because OrigData[CurOffset] is a UINT8, which gets promoted to
>     INT32, and that's when the bit-neg is applied
>
> - when the second word write is deemed necessary, then the
>   *BlockAddress* variable is bumped by 128 bytes out of laziness for
>   said second write -- and that is a *semantic wreck*. The BlockAddress
>   does not change *at all*; it's the start offset within the block that
>   increases by 128 bytes for the second word write.
>
> - The weird Exit and DoErase labels are fugly. The function should
>   either be split into two functions, or at least reorganized with "ifs"
>   such that this jumping is not necessary. Gotos are fine, but only for
>   error paths / cleanup on exit, not for business logic selection. IOW,
>   the main offender is DoErase.
>

Agree with all of these points.

>
> Then comments on the patch set:
>
> - In my opinion, the series should progress in opposite order. First
>   introduce a diagram (!), then refactor with the helper variables, and
>   then fix the bug. With the refactoring in place *first*, the bugfix
>   should be easier to understand. Then, potentially, generalize the code
>   to larger-than-two multiples of a word, for writes.
>
> - The first patch in the series is wrong.
>
>   In case we need not erase the whole flash block, we will want to write
>   one or two (consecutive) 128-byte "words". That is, 128 bytes, or 256
>   bytes. That means we need to read the exact same byte counts as well.
>
>   The *second* patch in the series actually seems to do this, with
>
>     End   = (Offset + *NumBytes + BOUNDARY_OF_32_WORDS) & ~BOUNDARY_OF_32_WORDS;
>
>   (This *in itself* would *much better* be written as follows:
>
>     End = ALIGN_VALUE (Offset + *NumBytes, P30_MAX_BUFFER_SIZE_IN_BYTES);
>
>   but I digress.)
>
>   However, the first patch still introduces:
>
>     (((Offset & BOUNDARY_OF_32_WORDS) + *NumBytes) | BOUNDARY_OF_32_WORDS) + 1
>
>   as the byte count for the read.
>
>   Unfortunately, the "saturation logic" (i.e., OR-ing 0x7F to the
>   exclusive end offset, for "seeking" to the end of the word), and then
>   adding 1, does not implement a correct "align-up" operation.
>
>   Consider
>
>     Offset == 0 && *NumBytes == 256
>
>   This circumstance is *valid* for the optimization path (and it is
>   correctly permitted by the top-most check).
>
>   But the expression introduced by patch#1 produces *384* for it, which
>   is wrong.
>
>   Similarly, given (for example)
>
>     Offset == 1 && *NumBytes == 127
>
>   the formula from patch#1 evaluates to 256.
>
>   The expression does not consider the case when the exclusive end
>   offset of the requested (logical write) is immediately at a word
>   boundary (i.e., a multiple of 128). In that case namely, saturating
>   with the bit-or, and adding 1, is wrong -- because in that case, no
>   additional block should be read at all.
>
>   So the first patch in the series replaces the *pre-series* bug with a
>   different (less harmful) bug, and then the second patch silently
>   *fixes* the replacement bug.
>
> - This is in fact the fundamental bug: the incorrect implementation of
>   the "align-up" operation with "saturate, then add 1". Both the
>   pre-series code, and the code in patch#1, contain this mistake.
>
>   The only thing that patch#1 changes is the *input*, to which the
>   (incorrect) operation is applied -- namely in patch#1, the *input*
>   changes from "NumBytes" to "exclusive end offset of the logical write,
>   relative to the start of the (double-)word".
>
>   That input change is in fact good (and *necessary*), but it's not
>   *sufficient*. The operation itself needs to be fixed.
>
> Summary:
>
> - please rewrite the series in the following order: refactoring, then
>   bugfix, then further armoring (additional sanity checks).
>
> - please only ever apply the bit-neg operator on values that are UINT32,
>   UINTN, or UINT64. Otherwise we get sign bit flipping, and that's
>   terrible. (Most people are not even aware of it happening.)
>
> - bit-fiddling should be kept to the absolute minimum. This means both a
>   need for helper variables (calculated as early as possible), and usage
>   of macros such as ALIGN_VALUE rather-than open-coded logic.
>
> It's possible that the refactoring in patch#2 is effectively impossible
> to do without fixing the *pre-series* bug at once. That's fine, as long
> as we point out the bug in the commit message.
>
> Importantly, the commit message should provide an actual (Offset,
> *NumBytes) tuple (an example) where the pre-series expression
>
>   (*NumBytes | BOUNDARY_OF_32_WORDS) + 1
>
> calculates a bogus byte count for the read.
>
> IOW, there are two things to highlight in the commit message:
>
> - round-up operation incorrectly implemented,
>
> - wrong input provided to the (already incorrect) round-up operation.
>

Thanks for taking the time to review this series as well as the existing code.

I agree with all of this, and I feel responsible for the current state
to some extent, so I will make time to get this fixed.

Gerd, if you are up for doing some of the work too and see a
meaningful split that would allow us to spread the load, feel free to
throw some of it my way. Otherwise, I will put it on my TODO list, and
I will get to it before the end of the month.


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

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

[Laszlo Ersek]

On 1/15/24 18:56, Ard Biesheuvel wrote:
> On Mon, 15 Jan 2024 at 11:21, Laszlo Ersek <lersek@redhat.com> wrote:
>>
>> On 1/12/24 12:37, 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.
>>>
>>> Gerd Hoffmann (4):
>>>   OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads
>>>   OvmfPkg/VirtNorFlashDxe: clarify block write logic
>>>   OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
>>>   OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
>>>
>>>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 33 +++++++++++------------
>>>  OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |  5 ++++
>>>  2 files changed, 21 insertions(+), 17 deletions(-)
>>>
>>
>> Looking at the original code makes me throw a fit (no offense -- I don't
>> know who wrote it, and I don't want to check).
>>
> 
> Hi Laszlo,
> 
> I am not the author of the original code, but I suppose I should take
> at least some of the blame here, having added some of the logic to
> reduce the number of MMIO accesses (which are disproportionately
> expensive under virtualization), and this is where the bug got
> introduced afaict.

... sorry about being needlessly harsh. If it's any excuse: in all such
cases I make a fully committed, honest effort to dig down to the "roots"
of the code, and the more I struggle to form a mental image, the more
annoyed/stressed I get. Comments and diagrams would definitely help with
my efforts, but just because I get annoyed during first analysis, that
is not sufficient reason to let that *leak* to the list. It's a
personality defect on my end. I'll keep working on it.

Laszlo



-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#113881): https://edk2.groups.io/g/devel/message/113881
Mute This Topic: https://groups.io/mt/103680930/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 0/4] OvmfPkg/VirtNorFlashDxe: fix corruption + misc small improvements

[Ard Biesheuvel]

On Tue, 16 Jan 2024 at 10:37, Laszlo Ersek <lersek@redhat.com> wrote:
>
> On 1/15/24 18:56, Ard Biesheuvel wrote:
> > On Mon, 15 Jan 2024 at 11:21, Laszlo Ersek <lersek@redhat.com> wrote:
> >>
> >> On 1/12/24 12:37, 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.
> >>>
> >>> Gerd Hoffmann (4):
> >>>   OvmfPkg/VirtNorFlashDxe: fix shadowbuffer reads
> >>>   OvmfPkg/VirtNorFlashDxe: clarify block write logic
> >>>   OvmfPkg/VirtNorFlashDxe: allow larger writes without block erase
> >>>   OvmfPkg/VirtNorFlashDxe: ValidateFvHeader: unwritten state is EOL too
> >>>
> >>>  OvmfPkg/VirtNorFlashDxe/VirtNorFlash.c    | 33 +++++++++++------------
> >>>  OvmfPkg/VirtNorFlashDxe/VirtNorFlashFvb.c |  5 ++++
> >>>  2 files changed, 21 insertions(+), 17 deletions(-)
> >>>
> >>
> >> Looking at the original code makes me throw a fit (no offense -- I don't
> >> know who wrote it, and I don't want to check).
> >>
> >
> > Hi Laszlo,
> >
> > I am not the author of the original code, but I suppose I should take
> > at least some of the blame here, having added some of the logic to
> > reduce the number of MMIO accesses (which are disproportionately
> > expensive under virtualization), and this is where the bug got
> > introduced afaict.
>
> ... sorry about being needlessly harsh. If it's any excuse: in all such
> cases I make a fully committed, honest effort to dig down to the "roots"
> of the code, and the more I struggle to form a mental image, the more
> annoyed/stressed I get. Comments and diagrams would definitely help with
> my efforts, but just because I get annoyed during first analysis, that
> is not sufficient reason to let that *leak* to the list. It's a
> personality defect on my end. I'll keep working on it.
>

Don't worry about it, really.

I don't mind unfiltered criticism from long-time collaborators as long
as it is constructive - email is such a lossy medium in terms of
subtext that I'd rather suffer a minor ego bruise than having to
unwrap layers of politeness to get at the real meaning.


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