[PATCH v3 11/33] hw/i386: refactor x86_bios_rom_init for reuse in confidential guest reset

Ani Sinha posted 33 patches 1 week, 6 days ago
Maintainers: Paolo Bonzini <pbonzini@redhat.com>, Eduardo Habkost <eduardo@habkost.net>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, "Philippe Mathieu-Daudé" <philmd@linaro.org>, Yanan Wang <wangyanan55@huawei.com>, Zhao Liu <zhao1.liu@intel.com>, "Maciej S. Szmigiero" <maciej.szmigiero@oracle.com>, Richard Henderson <richard.henderson@linaro.org>, "Michael S. Tsirkin" <mst@redhat.com>, David Woodhouse <dwmw2@infradead.org>, Paul Durrant <paul@xen.org>, Bernhard Beschow <shentey@gmail.com>, Alex Williamson <alex@shazbot.org>, "Cédric Le Goater" <clg@redhat.com>, Peter Xu <peterx@redhat.com>, Peter Maydell <peter.maydell@linaro.org>, Marcelo Tosatti <mtosatti@redhat.com>, Song Gao <gaosong@loongson.cn>, Huacai Chen <chenhuacai@kernel.org>, Aurelien Jarno <aurelien@aurel32.net>, Jiaxun Yang <jiaxun.yang@flygoat.com>, Aleksandar Rikalo <arikalo@gmail.com>, Nicholas Piggin <npiggin@gmail.com>, Harsh Prateek Bora <harshpb@linux.ibm.com>, Chinmay Rath <rathc@linux.ibm.com>, Palmer Dabbelt <palmer@dabbelt.com>, Alistair Francis <alistair.francis@wdc.com>, Weiwei Li <liwei1518@gmail.com>, Daniel Henrique Barboza <dbarboza@ventanamicro.com>, Liu Zhiwei <zhiwei_liu@linux.alibaba.com>, Halil Pasic <pasic@linux.ibm.com>, Christian Borntraeger <borntraeger@linux.ibm.com>, Eric Farman <farman@linux.ibm.com>, Matthew Rosato <mjrosato@linux.ibm.com>, Ilya Leoshkevich <iii@linux.ibm.com>, David Hildenbrand <david@kernel.org>, Thomas Huth <thuth@redhat.com>, Ani Sinha <anisinha@redhat.com>
[PATCH v3 11/33] hw/i386: refactor x86_bios_rom_init for reuse in confidential guest reset
Posted by Ani Sinha 1 week, 6 days ago
For confidential guests, bios image must be reinitialized upon reset. This
is because bios memory is encrypted and hence once the old confidential
kvm context is destroyed, it cannot be decrypted. It needs to be reinitilized.
In order to do that, this change refactors x86_bios_rom_init() code so that
parts of it can be called during confidential guest reset.

Signed-off-by: Ani Sinha <anisinha@redhat.com>
---
 hw/i386/x86-common.c  | 51 ++++++++++++++++++++++++++++++++-----------
 include/hw/i386/x86.h |  1 -
 2 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
index c1c9224039..4469b4e152 100644
--- a/hw/i386/x86-common.c
+++ b/hw/i386/x86-common.c
@@ -1024,17 +1024,11 @@ void x86_isa_bios_init(MemoryRegion *isa_bios, MemoryRegion *isa_memory,
     memory_region_set_readonly(isa_bios, read_only);
 }
 
-void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
-                       MemoryRegion *rom_memory, bool isapc_ram_fw)
+static int get_bios_size(X86MachineState *x86ms,
+                         const char *bios_name, char *filename)
 {
-    const char *bios_name;
-    char *filename;
     int bios_size;
-    ssize_t ret;
 
-    /* BIOS load */
-    bios_name = MACHINE(x86ms)->firmware ?: default_firmware;
-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
     if (filename) {
         bios_size = get_image_size(filename, NULL);
     } else {
@@ -1044,6 +1038,21 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
         (bios_size % 65536) != 0) {
         goto bios_error;
     }
+
+    return bios_size;
+
+ bios_error:
+    fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
+    exit(1);
+}
+
+static void load_bios_from_file(X86MachineState *x86ms, const char *bios_name,
+                                char *filename, int bios_size,
+                                bool isapc_ram_fw)
+{
+    ssize_t ret;
+
+    /* BIOS load */
     if (machine_require_guest_memfd(MACHINE(x86ms))) {
         memory_region_init_ram_guest_memfd(&x86ms->bios, NULL, "pc.bios",
                                            bios_size, &error_fatal);
@@ -1072,7 +1081,26 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
             goto bios_error;
         }
     }
-    g_free(filename);
+
+    return;
+
+ bios_error:
+    fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
+    exit(1);
+}
+
+void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
+                       MemoryRegion *rom_memory, bool isapc_ram_fw)
+{
+    int bios_size;
+    const char *bios_name;
+    char *filename;
+
+    bios_name = MACHINE(x86ms)->firmware ?: default_firmware;
+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
+
+    bios_size = get_bios_size(x86ms, bios_name, filename);
+    load_bios_from_file(x86ms, bios_name, filename, bios_size, isapc_ram_fw);
 
     if (!machine_require_guest_memfd(MACHINE(x86ms))) {
         /* map the last 128KB of the BIOS in ISA space */
@@ -1084,9 +1112,6 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
     memory_region_add_subregion(rom_memory,
                                 (uint32_t)(-bios_size),
                                 &x86ms->bios);
+    g_free(filename);
     return;
-
-bios_error:
-    fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
-    exit(1);
 }
diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
index 0dffba95f9..bfdf97640d 100644
--- a/include/hw/i386/x86.h
+++ b/include/hw/i386/x86.h
@@ -122,7 +122,6 @@ void x86_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
                                DeviceState *dev, Error **errp);
 void x86_cpu_unplug_cb(HotplugHandler *hotplug_dev,
                        DeviceState *dev, Error **errp);
-
 void x86_isa_bios_init(MemoryRegion *isa_bios, MemoryRegion *isa_memory,
                        MemoryRegion *bios, bool read_only);
 void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
-- 
2.42.0
Re: [PATCH v3 11/33] hw/i386: refactor x86_bios_rom_init for reuse in confidential guest reset
Posted by Bernhard Beschow 1 week, 6 days ago

Am 27. Januar 2026 05:15:39 UTC schrieb Ani Sinha <anisinha@redhat.com>:
>For confidential guests, bios image must be reinitialized upon reset. This
>is because bios memory is encrypted and hence once the old confidential
>kvm context is destroyed, it cannot be decrypted. It needs to be reinitilized.
>In order to do that, this change refactors x86_bios_rom_init() code so that
>parts of it can be called during confidential guest reset.
>
>Signed-off-by: Ani Sinha <anisinha@redhat.com>
>---
> hw/i386/x86-common.c  | 51 ++++++++++++++++++++++++++++++++-----------
> include/hw/i386/x86.h |  1 -
> 2 files changed, 38 insertions(+), 14 deletions(-)
>
>diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
>index c1c9224039..4469b4e152 100644
>--- a/hw/i386/x86-common.c
>+++ b/hw/i386/x86-common.c
>@@ -1024,17 +1024,11 @@ void x86_isa_bios_init(MemoryRegion *isa_bios, MemoryRegion *isa_memory,
>     memory_region_set_readonly(isa_bios, read_only);
> }
> 
>-void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
>-                       MemoryRegion *rom_memory, bool isapc_ram_fw)
>+static int get_bios_size(X86MachineState *x86ms,
>+                         const char *bios_name, char *filename)
> {
>-    const char *bios_name;
>-    char *filename;
>     int bios_size;
>-    ssize_t ret;
> 
>-    /* BIOS load */
>-    bios_name = MACHINE(x86ms)->firmware ?: default_firmware;
>-    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>     if (filename) {
>         bios_size = get_image_size(filename, NULL);
>     } else {
>@@ -1044,6 +1038,21 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
>         (bios_size % 65536) != 0) {
>         goto bios_error;
>     }
>+
>+    return bios_size;
>+
>+ bios_error:
>+    fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
>+    exit(1);
>+}
>+
>+static void load_bios_from_file(X86MachineState *x86ms, const char *bios_name,
>+                                char *filename, int bios_size,
>+                                bool isapc_ram_fw)
>+{
>+    ssize_t ret;
>+
>+    /* BIOS load */
>     if (machine_require_guest_memfd(MACHINE(x86ms))) {
>         memory_region_init_ram_guest_memfd(&x86ms->bios, NULL, "pc.bios",
>                                            bios_size, &error_fatal);
>@@ -1072,7 +1081,26 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
>             goto bios_error;
>         }
>     }
>-    g_free(filename);
>+
>+    return;
>+
>+ bios_error:
>+    fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
>+    exit(1);
>+}
>+
>+void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
>+                       MemoryRegion *rom_memory, bool isapc_ram_fw)
>+{
>+    int bios_size;
>+    const char *bios_name;
>+    char *filename;
>+
>+    bios_name = MACHINE(x86ms)->firmware ?: default_firmware;
>+    filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, bios_name);
>+
>+    bios_size = get_bios_size(x86ms, bios_name, filename);
>+    load_bios_from_file(x86ms, bios_name, filename, bios_size, isapc_ram_fw);
> 
>     if (!machine_require_guest_memfd(MACHINE(x86ms))) {
>         /* map the last 128KB of the BIOS in ISA space */
>@@ -1084,9 +1112,6 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
>     memory_region_add_subregion(rom_memory,
>                                 (uint32_t)(-bios_size),
>                                 &x86ms->bios);
>+    g_free(filename);

Doesn't the error path leak filename? Using g_autofree would prevent that. Even if the error path exits immediately today it may represent a latent bug for tomorrow (e.g. when porting to error API).

>     return;
>-
>-bios_error:
>-    fprintf(stderr, "qemu: could not load PC BIOS '%s'\n", bios_name);
>-    exit(1);
> }
>diff --git a/include/hw/i386/x86.h b/include/hw/i386/x86.h
>index 0dffba95f9..bfdf97640d 100644
>--- a/include/hw/i386/x86.h
>+++ b/include/hw/i386/x86.h
>@@ -122,7 +122,6 @@ void x86_cpu_unplug_request_cb(HotplugHandler *hotplug_dev,
>                                DeviceState *dev, Error **errp);
> void x86_cpu_unplug_cb(HotplugHandler *hotplug_dev,
>                        DeviceState *dev, Error **errp);
>-

This file contains whitespace changes only and breaks logic grouping of functions. I'd drop this change.

Best regards,
Bernhard

> void x86_isa_bios_init(MemoryRegion *isa_bios, MemoryRegion *isa_memory,
>                        MemoryRegion *bios, bool read_only);
> void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,