[PATCH 1/1] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection

[PATCH 1/1] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection

[Gerd Hoffmann]

Check whenever flash is actually writable.

Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
index 54f859de9ff9..a577aea55614 100644
--- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
+++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
@@ -114,9 +114,17 @@ QemuFlashDetected (
       DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as RAM\n"));
       *Ptr = OriginalUint8;
     } else if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
-      DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH\n"));
-      FlashDetected = TRUE;
-      *Ptr          = READ_ARRAY_CMD;
+      *Ptr       = WRITE_BYTE_CMD;
+      *Ptr       = OriginalUint8;
+      *Ptr       = READ_STATUS_CMD;
+      ProbeUint8 = *Ptr;
+      *Ptr       = READ_ARRAY_CMD;
+      if (ProbeUint8 & 0x10 /* programming error */) {
+        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH, write-protected\n"));
+      } else {
+        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH, writable\n"));
+        FlashDetected = TRUE;
+      }
     }
   }
 
-- 
2.40.0

Re: [PATCH 1/1] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection

[Ard Biesheuvel]

On Tue, 2 May 2023 at 07:59, Gerd Hoffmann <kraxel@redhat.com> wrote:
>
> Check whenever flash is actually writable.
>

This is a bit too terse. Could you explain why this is needed, and why
this approach is suitable?

> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> index 54f859de9ff9..a577aea55614 100644
> --- a/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> +++ b/OvmfPkg/QemuFlashFvbServicesRuntimeDxe/QemuFlash.c
> @@ -114,9 +114,17 @@ QemuFlashDetected (
>        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as RAM\n"));
>        *Ptr = OriginalUint8;
>      } else if (ProbeUint8 == CLEARED_ARRAY_STATUS) {
> -      DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH\n"));
> -      FlashDetected = TRUE;
> -      *Ptr          = READ_ARRAY_CMD;
> +      *Ptr       = WRITE_BYTE_CMD;
> +      *Ptr       = OriginalUint8;
> +      *Ptr       = READ_STATUS_CMD;
> +      ProbeUint8 = *Ptr;
> +      *Ptr       = READ_ARRAY_CMD;
> +      if (ProbeUint8 & 0x10 /* programming error */) {
> +        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH, write-protected\n"));
> +      } else {
> +        DEBUG ((DEBUG_INFO, "QemuFlashDetected => FD behaves as FLASH, writable\n"));
> +        FlashDetected = TRUE;
> +      }
>      }
>    }
>
> --
> 2.40.0
>

Re: [PATCH 1/1] OvmfPkg/QemuFlashFvbServicesRuntimeDxe: refine flash detection

[Gerd Hoffmann]

On Thu, May 25, 2023 at 05:53:27PM +0200, Ard Biesheuvel wrote:
> On Tue, 2 May 2023 at 07:59, Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > Check whenever flash is actually writable.
> >
> 
> This is a bit too terse. Could you explain why this is needed, and why
> this approach is suitable?

Flash can be write-protected in qemu (which is usually the case for
code).  In case the variable store flash block is configured read-only
ovmf wouldn't be able to store EFI variables there, so not setting up
fvb in that case (and fallhack to emulation) is the better option.
It'll avoid problems later due to flash writes failing.

The patch tries to write back the original value read earlier, so flash
content doesn't change in case the write succeeds.  But the status we
read back after the attempt to write will tell us whenever flash is
writable or not.

take care,
  Gerd