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

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

On 6/14/2024 4:48 PM, Gupta, Pankaj wrote:
> 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?

I'm asking why x86_isa_bios_init() cannot be called when machine 
requires guest memfd.

Please see my comment[1] to previous patch, patch 27, the two patches 
conflict with each other. The two patches did lack the clarification on 
the changes it made. sigh...

[1] 
https://lore.kernel.org/qemu-devel/ce895ad3-7a84-4af1-8927-6e85f60ef4f6@intel.com/

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