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

["Gupta, Pankaj" <pankaj.gupta@amd.com>]

On 6/14/2024 10:34 AM, Xiaoyao Li wrote:
> 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?

I could not understand your comment entirely. This condition is for non 
guest_memfd case? You expect something else?

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