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

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

[Gerd Hoffmann]

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.

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.1

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

[Oliver Steffen]

Quoting Gerd Hoffmann (2023-06-01 08:08:03)
> 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.

I like the term "fallhack", I'll keep that in mind for future use >,<

> 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.
>
> 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.1
>

- Oliver

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

[Ard Biesheuvel]

On Thu, 1 Jun 2023 at 10:22, Oliver Steffen <osteffen@redhat.com> wrote:
>
> Quoting Gerd Hoffmann (2023-06-01 08:08:03)
> > 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.
>
> I like the term "fallhack", I'll keep that in mind for future use >,<
>

+1

Merged as #4461

> > 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.
> >
> > 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.1
> >
>
> - Oliver
>