Normally, there is no need to perform firmware reconfiguration once the
virtal machine has started. Hence, currently ovmf firmware parsing happens only
once. However, if the firmware changes betweeen boots then reconfiguration needs
to happen again. Firmware can change if for example the guest brings its own
firmware bundle and installs it with the help of the hypervisor[1]. Therefore,
this change introduces a new api with which firmware configuration steps can
be forced again.
This is mostly refactoring work. No functional changes. CI pipeline does not
break with this change.

1) https://pretalx.com/kvm-forum-2024/talk/HJSKRQ/

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 hw/i386/pc_sysfw.c            | 25 +++++++++++++++++--------
 hw/i386/pc_sysfw_ovmf-stubs.c |  3 ++-
 hw/i386/pc_sysfw_ovmf.c       |  5 +++--
 include/hw/i386/pc.h          |  3 ++-
 include/hw/i386/x86.h         |  1 +
 5 files changed, 25 insertions(+), 12 deletions(-)

diff --git a/hw/i386/pc_sysfw.c b/hw/i386/pc_sysfw.c
index 1eeb58ab37..0b9913d7b9 100644
--- a/hw/i386/pc_sysfw.c
+++ b/hw/i386/pc_sysfw.c
@@ -258,16 +258,9 @@ void pc_system_firmware_init(PCMachineState *pcms,
     pc_system_flash_cleanup_unused(pcms);
 }
 
-void x86_firmware_configure(hwaddr gpa, void *ptr, int size)
+static void x86_firmware_configure_sev(hwaddr gpa, void *ptr, int size)
 {
     int ret;
-
-    /*
-     * OVMF places a GUIDed structures in the flash, so
-     * search for them
-     */
-    pc_system_parse_ovmf_flash(ptr, size);
-
     if (sev_enabled()) {
 
         /* Copy the SEV metadata table (if it exists) */
@@ -282,3 +275,19 @@ void x86_firmware_configure(hwaddr gpa, void *ptr, int size)
         sev_encrypt_flash(gpa, ptr, size, &error_fatal);
     }
 }
+
+void x86_firmware_configure(hwaddr gpa, void *ptr, int size)
+{
+    /*
+     * OVMF places a GUIDed structures in the flash, so
+     * search for them
+     */
+    pc_system_parse_ovmf_flash(ptr, size, false);
+    x86_firmware_configure_sev(gpa, ptr, size);
+}
+
+void x86_firmware_reconfigure(hwaddr gpa, void *ptr, int size)
+{
+    pc_system_parse_ovmf_flash(ptr, size, true);
+    x86_firmware_configure_sev(gpa, ptr, size);
+}
diff --git a/hw/i386/pc_sysfw_ovmf-stubs.c b/hw/i386/pc_sysfw_ovmf-stubs.c
index aabe78b271..a8c0c265d7 100644
--- a/hw/i386/pc_sysfw_ovmf-stubs.c
+++ b/hw/i386/pc_sysfw_ovmf-stubs.c
@@ -20,7 +20,8 @@ bool pc_system_ovmf_table_find(const char *entry, uint8_t **data, int *data_len)
     g_assert_not_reached();
 }
 
-void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size)
+void pc_system_parse_ovmf_flash(uint8_t *flash_ptr,
+                                size_t flash_size, bool force)
 {
     g_assert_not_reached();
 }
diff --git a/hw/i386/pc_sysfw_ovmf.c b/hw/i386/pc_sysfw_ovmf.c
index 07a4c267fa..7d54622771 100644
--- a/hw/i386/pc_sysfw_ovmf.c
+++ b/hw/i386/pc_sysfw_ovmf.c
@@ -36,14 +36,15 @@ static bool ovmf_flash_parsed;
 static uint8_t *ovmf_table;
 static int ovmf_table_len;
 
-void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size)
+void pc_system_parse_ovmf_flash(uint8_t *flash_ptr,
+                                size_t flash_size, bool force)
 {
     uint8_t *ptr;
     QemuUUID guid;
     int tot_len;
 
     /* should only be called once */
-    if (ovmf_flash_parsed) {
+    if (ovmf_flash_parsed && !force) {
         return;
     }
 
diff --git a/include/hw/i386/pc.h b/include/hw/i386/pc.h
index 103b54301f..769f1361ce 100644
--- a/include/hw/i386/pc.h
+++ b/include/hw/i386/pc.h
@@ -210,7 +210,8 @@ void pc_system_flash_cleanup_unused(PCMachineState *pcms);
 void pc_system_firmware_init(PCMachineState *pcms, MemoryRegion *rom_memory);
 bool pc_system_ovmf_table_find(const char *entry, uint8_t **data,
                                int *data_len);
-void pc_system_parse_ovmf_flash(uint8_t *flash_ptr, size_t flash_size);
+void pc_system_parse_ovmf_flash(uint8_t *flash_ptr,
+                                size_t flash_size, bool force);
 
 /* sgx.c */
 void pc_machine_init_sgx_epc(PCMachineState *pcms);
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index d43cb3908e..18c0d6851a 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -155,5 +155,6 @@ DeviceState *ioapic_init_secondary(GSIState *gsi_state);
 
 /* pc_sysfw.c */
 void x86_firmware_configure(hwaddr gpa, void *ptr, int size);
+void x86_firmware_reconfigure(hwaddr gpa, void *ptr, int size);
 
 #endif
-- 
2.42.0

  Hi,

>      /* should only be called once */
> -    if (ovmf_flash_parsed) {
> +    if (ovmf_flash_parsed && !force) {

I think it makes more sense to clear ovmf_flash_parsed when replacing
the firmware (instead of adding the force override).

take care,
  Gerd

On Mon, Feb 24, 2025 at 9:01 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
>
>   Hi,
>
> >      /* should only be called once */
> > -    if (ovmf_flash_parsed) {
> > +    if (ovmf_flash_parsed && !force) {
>
> I think it makes more sense to clear ovmf_flash_parsed when replacing
> the firmware (instead of adding the force override).

I thought about that but wondered if that is an internal
variable/implementation specific detail which should not be exposed
outside.

>
> take care,
>   Gerd
>

On Tue, Feb 25, 2025 at 09:10:40AM +0530, Ani Sinha wrote:
> On Mon, Feb 24, 2025 at 9:01 PM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> >   Hi,
> >
> > >      /* should only be called once */
> > > -    if (ovmf_flash_parsed) {
> > > +    if (ovmf_flash_parsed && !force) {
> >
> > I think it makes more sense to clear ovmf_flash_parsed when replacing
> > the firmware (instead of adding the force override).
> 
> I thought about that but wondered if that is an internal
> variable/implementation specific detail which should not be exposed
> outside.

You don't have to expose the variable directly, you can also provide a
helper function to clear it.  Best with a name which documents the
purpose.

take care,
  Gerd