Re: [PATCH v4 28/31] hw/i386: Add support for loading BIOS using guest_memfd

[Xiaoyao Li <xiaoyao.li@intel.com>]

On 5/30/2024 7:16 PM, Pankaj Gupta wrote:
> From: Michael Roth <michael.roth@amd.com>
> 
> When guest_memfd is enabled, the BIOS is generally part of the initial
> encrypted guest image and will be accessed as private guest memory. Add
> the necessary changes to set up the associated RAM region with a
> guest_memfd backend to allow for this.
> 
> Current support centers around using -bios to load the BIOS data.
> Support for loading the BIOS via pflash requires additional enablement
> since those interfaces rely on the use of ROM memory regions which make
> use of the KVM_MEM_READONLY memslot flag, which is not supported for
> guest_memfd-backed memslots.
> 
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> Signed-off-by: Pankaj Gupta <pankaj.gupta@amd.com>
> ---
>   hw/i386/x86-common.c | 22 ++++++++++++++++------
>   1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/i386/x86-common.c b/hw/i386/x86-common.c
> index f41cb0a6a8..059de65f36 100644
> --- a/hw/i386/x86-common.c
> +++ b/hw/i386/x86-common.c
> @@ -999,10 +999,18 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
>       }
>       if (bios_size <= 0 ||
>           (bios_size % 65536) != 0) {
> -        goto bios_error;
> +        if (!machine_require_guest_memfd(MACHINE(x86ms))) {
> +                g_warning("%s: Unaligned BIOS size %d", __func__, bios_size);
> +                goto bios_error;
> +        }
> +    }
> +    if (machine_require_guest_memfd(MACHINE(x86ms))) {
> +        memory_region_init_ram_guest_memfd(&x86ms->bios, NULL, "pc.bios",
> +                                           bios_size, &error_fatal);
> +    } else {
> +        memory_region_init_ram(&x86ms->bios, NULL, "pc.bios",
> +                               bios_size, &error_fatal);
>       }
> -    memory_region_init_ram(&x86ms->bios, NULL, "pc.bios", bios_size,
> -                           &error_fatal);
>       if (sev_enabled()) {
>           /*
>            * The concept of a "reset" simply doesn't exist for
> @@ -1023,9 +1031,11 @@ void x86_bios_rom_init(X86MachineState *x86ms, const char *default_firmware,
>       }
>       g_free(filename);
>   
> -    /* map the last 128KB of the BIOS in ISA space */
> -    x86_isa_bios_init(&x86ms->isa_bios, rom_memory, &x86ms->bios,
> -                      !isapc_ram_fw);
> +    if (!machine_require_guest_memfd(MACHINE(x86ms))) {
> +        /* map the last 128KB of the BIOS in ISA space */
> +        x86_isa_bios_init(&x86ms->isa_bios, rom_memory, &x86ms->bios,
> +                          !isapc_ram_fw);
> +    }

Could anyone explain to me why above change is related to this patch and 
why need it?

because inside x86_isa_bios_init(), the alias isa_bios is set to 
read_only while guest_memfd doesn't support readonly?

>       /* map all the bios at the top of memory */
>       memory_region_add_subregion(rom_memory,