[PATCH] hw/i386: Add unaccepted memory configuration

[PATCH] hw/i386: Add unaccepted memory configuration

[Dionna Glaze]

For SEV-SNP, an OS is "SEV-SNP capable" without supporting this UEFI
v2.9 memory type. In order for OVMF to be able to avoid pre-validating
potentially hundreds of gibibytes of data before booting, it needs to
know if the guest OS can support its use of the new type of memory in
the memory map.

Cc: Xu, Min M <min.m.xu@intel.com>
Cc: Xiaoyao Li <xiaoyao.li@intel.com>
Cc: Thomas Lendacky <Thomas.Lendacky@amd.com>
Cc: Gerd Hoffman <kraxel@redhat.com>
Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 hw/core/machine.c   | 33 +++++++++++++++++++++++++++++++++
 hw/i386/fw_cfg.c    |  5 +++++
 include/hw/boards.h |  1 +
 3 files changed, 39 insertions(+)

diff --git a/hw/core/machine.c b/hw/core/machine.c
index c53548d0b1..d2b9513951 100644
--- a/hw/core/machine.c
+++ b/hw/core/machine.c
@@ -263,6 +263,15 @@ GlobalProperty hw_compat_2_1[] = {
 };
 const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
 
+static QEnumLookup memory_acceptance_lookup = {
+    .array = (const char *const[]) {
+        "default",
+        "true",
+        "false",
+    },
+    .size = 3,
+};
+
 MachineState *current_machine;
 
 static char *machine_get_kernel(Object *obj, Error **errp)
@@ -502,6 +511,20 @@ static void machine_check_confidential_guest_support(const Object *obj,
      */
 }
 
+static int machine_get_accept_all_memory(Object *obj, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    return ms->accept_all_memory;
+}
+
+static void machine_set_accept_all_memory(Object *obj, int value, Error **errp)
+{
+    MachineState *ms = MACHINE(obj);
+
+    ms->accept_all_memory = value;
+}
+
 static bool machine_get_nvdimm(Object *obj, Error **errp)
 {
     MachineState *ms = MACHINE(obj);
@@ -1022,6 +1045,15 @@ static void machine_class_init(ObjectClass *oc, void *data)
     object_class_property_set_description(oc, "confidential-guest-support",
                                           "Set confidential guest scheme to support");
 
+    object_class_property_add_enum(oc, "accept-all-memory",
+                                   "MemoryAcceptance",
+                                   &memory_acceptance_lookup,
+        machine_get_accept_all_memory, machine_set_accept_all_memory);
+    object_class_property_set_description(
+        oc, "accept-all-memory",
+        "false: Accept all memory, true: Accept up to 4G and leave the rest unaccepted (UEFI"
+        " v2.9 memory type), default: default firmware behavior.");
+
     /* For compatibility */
     object_class_property_add_str(oc, "memory-encryption",
         machine_get_memory_encryption, machine_set_memory_encryption);
@@ -1072,6 +1104,7 @@ static void machine_initfn(Object *obj)
     ms->kernel_cmdline = g_strdup("");
     ms->ram_size = mc->default_ram_size;
     ms->maxram_size = mc->default_ram_size;
+    ms->accept_all_memory = 0;
 
     if (mc->nvdimm_supported) {
         Object *obj = OBJECT(ms);
diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
index a283785a8d..96164994f8 100644
--- a/hw/i386/fw_cfg.c
+++ b/hw/i386/fw_cfg.c
@@ -131,6 +131,11 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
                      &e820_reserve, sizeof(e820_reserve));
     fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
                     sizeof(struct e820_entry) * e820_get_num_entries());
+    if (ms->accept_all_memory) {
+        bool accept_all = ms->accept_all_memory == 1;
+        fw_cfg_add_file(fw_cfg, "opt/ovmf/AcceptAllMemory",
+                        &accept_all, sizeof(accept_all));
+    }
 
     fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
     /* allocate memory for the NUMA channel: one (64bit) word for the number
diff --git a/include/hw/boards.h b/include/hw/boards.h
index fa57bac4fb..eaf73498c4 100644
--- a/include/hw/boards.h
+++ b/include/hw/boards.h
@@ -362,6 +362,7 @@ struct MachineState {
     struct NVDIMMState *nvdimms_state;
     struct CXLState *cxl_devices_state;
     struct NumaState *numa_state;
+    int accept_all_memory;
 };
 
 #define DEFINE_MACHINE(namestr, machine_initfn) \
-- 
2.37.0.rc0.104.g0611611a94-goog

Re: [PATCH] hw/i386: Add unaccepted memory configuration

[Gerd Hoffman]

On Mon, Jun 20, 2022 at 10:33:00PM +0000, Dionna Glaze wrote:
> For SEV-SNP, an OS is "SEV-SNP capable" without supporting this UEFI
> v2.9 memory type. In order for OVMF to be able to avoid pre-validating
> potentially hundreds of gibibytes of data before booting, it needs to
> know if the guest OS can support its use of the new type of memory in
> the memory map.

I think this should be wired up via sev-guest object (see
SevGuestProperties in qapi/qom.json and target/i386/sev.c),
i.e.

qemu -object sev-guest,accept-all-memory=true,$args

(and likewise for -object tdx-guest once merged).

take care,
  Gerd

Re: [PATCH] hw/i386: Add unaccepted memory configuration

[Michael S. Tsirkin]

On Tue, Jun 21, 2022 at 07:37:02AM +0200, Gerd Hoffman wrote:
> On Mon, Jun 20, 2022 at 10:33:00PM +0000, Dionna Glaze wrote:
> > For SEV-SNP, an OS is "SEV-SNP capable" without supporting this UEFI
> > v2.9 memory type. In order for OVMF to be able to avoid pre-validating
> > potentially hundreds of gibibytes of data before booting, it needs to
> > know if the guest OS can support its use of the new type of memory in
> > the memory map.
> 
> I think this should be wired up via sev-guest object (see
> SevGuestProperties in qapi/qom.json and target/i386/sev.c),
> i.e.
> 
> qemu -object sev-guest,accept-all-memory=true,$args
> 
> (and likewise for -object tdx-guest once merged).
> 
> take care,
>   Gerd

Right. As written the patch would allow the flag without SEV-SNP too -
but does it make any sense outside SEV-SNP? It's better not to allow
flag combinations that make no sense since they tend to
become part of ABI that we then need to support.

-- 
MST

Re: [PATCH] hw/i386: Add unaccepted memory configuration

[Gupta, Pankaj]

> For SEV-SNP, an OS is "SEV-SNP capable" without supporting this UEFI
> v2.9 memory type. In order for OVMF to be able to avoid pre-validating
> potentially hundreds of gibibytes of data before booting, it needs to
> know if the guest OS can support its use of the new type of memory in
> the memory map >
> Cc: Xu, Min M <min.m.xu@intel.com>
> Cc: Xiaoyao Li <xiaoyao.li@intel.com>
> Cc: Thomas Lendacky <Thomas.Lendacky@amd.com>
> Cc: Gerd Hoffman <kraxel@redhat.com>
> Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
> ---
>   hw/core/machine.c   | 33 +++++++++++++++++++++++++++++++++
>   hw/i386/fw_cfg.c    |  5 +++++
>   include/hw/boards.h |  1 +
>   3 files changed, 39 insertions(+)
> 
> diff --git a/hw/core/machine.c b/hw/core/machine.c
> index c53548d0b1..d2b9513951 100644
> --- a/hw/core/machine.c
> +++ b/hw/core/machine.c
> @@ -263,6 +263,15 @@ GlobalProperty hw_compat_2_1[] = {
>   };
>   const size_t hw_compat_2_1_len = G_N_ELEMENTS(hw_compat_2_1);
>   
> +static QEnumLookup memory_acceptance_lookup = {
> +    .array = (const char *const[]) {
> +        "default",
> +        "true",
> +        "false",
> +    },
> +    .size = 3,
> +};
> +
>   MachineState *current_machine;
>   
>   static char *machine_get_kernel(Object *obj, Error **errp)
> @@ -502,6 +511,20 @@ static void machine_check_confidential_guest_support(const Object *obj,
>        */
>   }
>   
> +static int machine_get_accept_all_memory(Object *obj, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    return ms->accept_all_memory;
> +}
> +
> +static void machine_set_accept_all_memory(Object *obj, int value, Error **errp)
> +{
> +    MachineState *ms = MACHINE(obj);
> +
> +    ms->accept_all_memory = value;
> +}
> +
>   static bool machine_get_nvdimm(Object *obj, Error **errp)
>   {
>       MachineState *ms = MACHINE(obj);
> @@ -1022,6 +1045,15 @@ static void machine_class_init(ObjectClass *oc, void *data)
>       object_class_property_set_description(oc, "confidential-guest-support",
>                                             "Set confidential guest scheme to support");
>   
> +    object_class_property_add_enum(oc, "accept-all-memory",
> +                                   "MemoryAcceptance",
> +                                   &memory_acceptance_lookup,
> +        machine_get_accept_all_memory, machine_set_accept_all_memory);
> +    object_class_property_set_description(
> +        oc, "accept-all-memory",
> +        "false: Accept all memory, true: Accept up to 4G and leave the rest unaccepted (UEFI"
> +        " v2.9 memory type), default: default firmware behavior.");

AFAIU 'true' is the behavior you are proposing with your EFI changes?
Saying that what's the difference between 'false' & 'default' wrt EFI
firmware? Just wondering do we need default?

> +
>       /* For compatibility */
>       object_class_property_add_str(oc, "memory-encryption",
>           machine_get_memory_encryption, machine_set_memory_encryption);
> @@ -1072,6 +1104,7 @@ static void machine_initfn(Object *obj)
>       ms->kernel_cmdline = g_strdup("");
>       ms->ram_size = mc->default_ram_size;
>       ms->maxram_size = mc->default_ram_size;
> +    ms->accept_all_memory = 0;
>   
>       if (mc->nvdimm_supported) {
>           Object *obj = OBJECT(ms);
> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c
> index a283785a8d..96164994f8 100644
> --- a/hw/i386/fw_cfg.c
> +++ b/hw/i386/fw_cfg.c
> @@ -131,6 +131,11 @@ FWCfgState *fw_cfg_arch_create(MachineState *ms,
>                        &e820_reserve, sizeof(e820_reserve));
>       fw_cfg_add_file(fw_cfg, "etc/e820", e820_table,
>                       sizeof(struct e820_entry) * e820_get_num_entries());
> +    if (ms->accept_all_memory) { > +        bool accept_all = ms->accept_all_memory == 1> + 
fw_cfg_add_file(fw_cfg, "opt/ovmf/AcceptAllMemory",
> +                        &accept_all, sizeof(accept_all));
> +    }
>   
>       fw_cfg_add_bytes(fw_cfg, FW_CFG_HPET, &hpet_cfg, sizeof(hpet_cfg));
>       /* allocate memory for the NUMA channel: one (64bit) word for the number
> diff --git a/include/hw/boards.h b/include/hw/boards.h
> index fa57bac4fb..eaf73498c4 100644
> --- a/include/hw/boards.h
> +++ b/include/hw/boards.h
> @@ -362,6 +362,7 @@ struct MachineState {
>       struct NVDIMMState *nvdimms_state;
>       struct CXLState *cxl_devices_state;
>       struct NumaState *numa_state;
> +    int accept_all_memory; >   };
>   
>   #define DEFINE_MACHINE(namestr, machine_initfn) \

Re: [PATCH] hw/i386: Add unaccepted memory configuration

[Gerd Hoffman]

  Hi,

> AFAIU 'true' is the behavior you are proposing with your EFI changes?
> Saying that what's the difference between 'false' & 'default' wrt EFI
> firmware? Just wondering do we need default?

true/false will force the one or the other no matter what.

'default' allows the firmware to choose depending on various factors,
for example have cc-specific build variants have a different default
behavior than the generic builds.

It also keeps the door open to change default behavior in the future.
One reasonable approach would be to start with firmware accepting all
memory by default, wait until support for unaccepted memory has found
its way into linux distro kernels, then flip the default to pass
unaccepted memory to the linux kernel.

In case the uefi boot service spec gets updated to allow negotiating
unaccepted memory support automatically this can be used easily by
making that the firmware's default behavior.

take care,
  Gerd

Re: [PATCH] hw/i386: Add unaccepted memory configuration

[Gupta, Pankaj]

Hi Gerd,

>    Hi,
> 
>> AFAIU 'true' is the behavior you are proposing with your EFI changes?
>> Saying that what's the difference between 'false' & 'default' wrt EFI
>> firmware? Just wondering do we need default?
> 
> true/false will force the one or the other no matter what.
> 
> 'default' allows the firmware to choose depending on various factors,
> for example have cc-specific build variants have a different default
> behavior than the generic builds.
> 
> It also keeps the door open to change default behavior in the future.
> One reasonable approach would be to start with firmware accepting all
> memory by default, wait until support for unaccepted memory has found
> its way into linux distro kernels, then flip the default to pass
> unaccepted memory to the linux kernel.
> 
> In case the uefi boot service spec gets updated to allow negotiating
> unaccepted memory support automatically this can be used easily by
> making that the firmware's default behavior.

Fair. Also, it would be interesting to see the right future combination
of {un/a}cceptable boot vs runtime aceptable memory and also support in 
uefi specs.

o.k to keep placeholder for now but I still see, might have to keep the 
interface extensible, maybe something like bit level negotiations
than true/false/default etc. But for now I don't have any strong opinion 
or ideas and have to catch up from uefi side.

Thank you for explaining.

Best regards,
Pankaj