diff mbox series

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

Message ID 20240530111643.1091816-29-pankaj.gupta@amd.com (mailing list archive)
State New
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) support | expand

Commit Message

Gupta, Pankaj May 30, 2024, 11:16 a.m. UTC 
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(-)

Comments

Paolo Bonzini May 31, 2024, 11:22 a.m. UTC | #1 
On Thu, May 30, 2024 at 1:17 PM Pankaj Gupta <pankaj.gupta@amd.com> wrote:
>      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;
> +        }

Why is this not needed for SEV-SNP? (The bios_size <= 0 case
definitely should be an error).

I'll just drop this change.

> +    }
> +    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);
> +    }
>
>      /* map all the bios at the top of memory */
>      memory_region_add_subregion(rom_memory,
> --
> 2.34.1
>

On Thu, May 30, 2024 at 1:17 PM Pankaj Gupta <pankaj.gupta@amd.com> 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);
> +    }
>
>      /* map all the bios at the top of memory */
>      memory_region_add_subregion(rom_memory,
> --
> 2.34.1
>
Xiaoyao Li June 14, 2024, 8:34 a.m. UTC | #2 
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,
Gupta, Pankaj June 14, 2024, 8:48 a.m. UTC | #3 
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,
>
Xiaoyao Li June 14, 2024, 9:03 a.m. UTC | #4 
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,
>>
>
diff mbox series

Patch

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);
+    }
 
     /* map all the bios at the top of memory */
     memory_region_add_subregion(rom_memory,