[PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Dionna Glaze]

This patch depends on Kirill A. Shutemov's series

[PATCHv8 00/14] mm, x86/cc: Implement support for unaccepted memory

The UEFI v2.9 specification includes a new memory type to be used in
environments where the OS must accept memory that is provided from its
host. Before the introduction of this memory type, all memory was
accepted eagerly in the firmware. In order for the firmware to safely
stop accepting memory on the OS's behalf, the OS must affirmatively
indicate support to the firmware.

Enabling unaccepted memory requires calling a 0-argument enablement
protocol before ExitBootServices. This call is only made if the kernel
is compiled with UNACCEPTED_MEMORY=y

The naming of the protocol guid is dependent on the standardization of
the protocol, which is being discussed. Acceptance is contingent on
the kernel community's approval.

Cc: Ard Biescheuvel <ardb@kernel.org>
Cc: "Min M. Xu" <min.m.xu@intel.org>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Tom Lendacky <Thomas.Lendacky@amd.com>
Cc: Jiewen Yao <jiewen.yao@intel.com>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
Cc: Dave Hansen <dave.hansen@linux.intel.com>

Signed-off-by: Dionna Glaze <dionnaglaze@google.com>
---
 drivers/firmware/efi/libstub/x86-stub.c | 37 +++++++++++++++++++++++++
 include/linux/efi.h                     |  1 +
 2 files changed, 38 insertions(+)

diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index a0bfd31358ba..abf31e5ade55 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -26,6 +26,17 @@ const efi_dxe_services_table_t *efi_dxe_table;
 u32 image_offset __section(".data");
 static efi_loaded_image_t *image = NULL;
 
+typedef union memory_acceptance_protocol memory_acceptance_protocol_t;
+union memory_acceptance_protocol {
+	struct {
+		efi_status_t (__efiapi *allow_unaccepted_memory)(
+			memory_acceptance_protocol_t *);
+	};
+	struct {
+		u32 allow_unaccepted_memory;
+	} mixed_mode;
+};
+
 static efi_status_t
 preserve_pci_rom_image(efi_pci_io_protocol_t *pci, struct pci_setup_rom **__rom)
 {
@@ -310,6 +321,30 @@ setup_memory_protection(unsigned long image_base, unsigned long image_size)
 #endif
 }
 
+
+static void setup_unaccepted_memory(void)
+{
+	efi_guid_t mem_acceptance_proto = EFI_MEMORY_ACCEPTANCE_PROTOCOL_GUID;
+	memory_acceptance_protocol_t *proto;
+	efi_status_t status;
+
+	if (!IS_ENABLED(CONFIG_UNACCEPTED_MEMORY))
+		return;
+
+	/*
+	 * Enable unaccepted memory before calling exit boot services in order
+	 * for the UEFI to not accept all memory on EBS.
+	 */
+	status = efi_bs_call(locate_protocol, &mem_acceptance_proto, NULL,
+			     (void **)&proto);
+	if (status != EFI_SUCCESS)
+		return;
+
+	status = efi_call_proto(proto, allow_unaccepted_memory);
+	if (status != EFI_SUCCESS)
+		efi_err("Memory acceptance protocol failed\n");
+}
+
 static const efi_char16_t apple[] = L"Apple";
 
 static void setup_quirks(struct boot_params *boot_params,
@@ -899,6 +934,8 @@ asmlinkage unsigned long efi_main(efi_handle_t handle,
 
 	setup_quirks(boot_params, bzimage_addr, buffer_end - buffer_start);
 
+	setup_unaccepted_memory();
+
 	status = exit_boot(boot_params, handle);
 	if (status != EFI_SUCCESS) {
 		efi_err("exit_boot() failed!\n");
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 4b27519143f5..bfc0e4f2aba5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -391,6 +391,7 @@ void efi_native_runtime_setup(void);
 #define EFI_RT_PROPERTIES_TABLE_GUID		EFI_GUID(0xeb66918a, 0x7eef, 0x402a,  0x84, 0x2e, 0x93, 0x1d, 0x21, 0xc3, 0x8a, 0xe9)
 #define EFI_DXE_SERVICES_TABLE_GUID		EFI_GUID(0x05ad34ba, 0x6f02, 0x4214,  0x95, 0x2e, 0x4d, 0xa0, 0x39, 0x8e, 0x2b, 0xb9)
 #define EFI_SMBIOS_PROTOCOL_GUID		EFI_GUID(0x03583ff6, 0xcb36, 0x4940,  0x94, 0x7e, 0xb9, 0xb3, 0x9f, 0x4a, 0xfa, 0xf7)
+#define EFI_MEMORY_ACCEPTANCE_PROTOCOL_GUID	EFI_GUID(0xc5a010fe, 0x38a7, 0x4531,  0x8a, 0x4a, 0x05, 0x00, 0xd2, 0xfd, 0x16, 0x49)
 
 #define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596,  0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
 #define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300,  0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
-- 
2.39.0.314.g84b9a713c41-goog

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Kirill A. Shutemov]

On Fri, Jan 13, 2023 at 09:29:26PM +0000, Dionna Glaze wrote:
> This patch depends on Kirill A. Shutemov's series
> 
> [PATCHv8 00/14] mm, x86/cc: Implement support for unaccepted memory
> 
> The UEFI v2.9 specification includes a new memory type to be used in
> environments where the OS must accept memory that is provided from its
> host. Before the introduction of this memory type, all memory was
> accepted eagerly in the firmware. In order for the firmware to safely
> stop accepting memory on the OS's behalf, the OS must affirmatively
> indicate support to the firmware.

I think it is a bad idea.

This approach breaks use case with a bootloader between BIOS and OS.
As the bootloader does ExitBootServices() it has to make the call on
behalf of OS when it has no idea if the OS supports unaccepted.

Note that kexec is such use-case: original kernel has to make a decision
on whether it is okay to leave some memory unaccepted for the new kernel.

And we add this protocol to address very temporary problem: once
unaccepted memory support get upstream it is just a dead weight.

Let's not do this.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Gerd Hoffmann]

On Sat, Jan 14, 2023 at 01:20:24AM +0300, Kirill A. Shutemov wrote:
> On Fri, Jan 13, 2023 at 09:29:26PM +0000, Dionna Glaze wrote:
> > This patch depends on Kirill A. Shutemov's series
> > 
> > [PATCHv8 00/14] mm, x86/cc: Implement support for unaccepted memory
> > 
> > The UEFI v2.9 specification includes a new memory type to be used in
> > environments where the OS must accept memory that is provided from its
> > host. Before the introduction of this memory type, all memory was
> > accepted eagerly in the firmware. In order for the firmware to safely
> > stop accepting memory on the OS's behalf, the OS must affirmatively
> > indicate support to the firmware.
> 
> I think it is a bad idea.
> 
> This approach breaks use case with a bootloader between BIOS and OS.
> As the bootloader does ExitBootServices() it has to make the call on
> behalf of OS when it has no idea if the OS supports unaccepted.

Nothing breaks, it'll error on the safe side.  If the protocol callback
is not called the firmware will simply accept all memory.  The guest OS
will only see unaccepted memory if it explicitly asked for it (assuming
the firmware wants know to support both cases, of course the firmware
could also enforce the one or the other and just not offer the
protocol).

> Note that kexec is such use-case: original kernel has to make a
> decision on whether it is okay to leave some memory unaccepted for the
> new kernel.

Not sure what you are trying to tell.  The kexec case doesn't go
through the efi stub anyway.

> And we add this protocol to address very temporary problem: once
> unaccepted memory support get upstream it is just a dead weight.

Maybe, maybe not.  unaccepted memory support has a Kconfig switch after
all.  If we figure in 3-5 years that all distros have enabled it anyway
we can drop it again.  For the transition period it will surely be
useful.

take care,
  Gerd

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Kirill A. Shutemov]

On Mon, Jan 16, 2023 at 11:56:48AM +0100, Gerd Hoffmann wrote:
> On Sat, Jan 14, 2023 at 01:20:24AM +0300, Kirill A. Shutemov wrote:
> > On Fri, Jan 13, 2023 at 09:29:26PM +0000, Dionna Glaze wrote:
> > > This patch depends on Kirill A. Shutemov's series
> > > 
> > > [PATCHv8 00/14] mm, x86/cc: Implement support for unaccepted memory
> > > 
> > > The UEFI v2.9 specification includes a new memory type to be used in
> > > environments where the OS must accept memory that is provided from its
> > > host. Before the introduction of this memory type, all memory was
> > > accepted eagerly in the firmware. In order for the firmware to safely
> > > stop accepting memory on the OS's behalf, the OS must affirmatively
> > > indicate support to the firmware.
> > 
> > I think it is a bad idea.
> > 
> > This approach breaks use case with a bootloader between BIOS and OS.
> > As the bootloader does ExitBootServices() it has to make the call on
> > behalf of OS when it has no idea if the OS supports unaccepted.
> 
> Nothing breaks, it'll error on the safe side.  If the protocol callback
> is not called the firmware will simply accept all memory.  The guest OS
> will only see unaccepted memory if it explicitly asked for it (assuming
> the firmware wants know to support both cases, of course the firmware
> could also enforce the one or the other and just not offer the
> protocol).

How bootloader suppose to know if OS will ask for unaccepted memory?
It can't. It means the use-case with bootloader cannot ever use
unaccepted memory. That's broken design.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Ard Biesheuvel]

On Mon, 16 Jan 2023 at 13:31, Kirill A. Shutemov <kirill@shutemov.name> wrote:
>
> On Mon, Jan 16, 2023 at 11:56:48AM +0100, Gerd Hoffmann wrote:
> > On Sat, Jan 14, 2023 at 01:20:24AM +0300, Kirill A. Shutemov wrote:
> > > On Fri, Jan 13, 2023 at 09:29:26PM +0000, Dionna Glaze wrote:
> > > > This patch depends on Kirill A. Shutemov's series
> > > >
> > > > [PATCHv8 00/14] mm, x86/cc: Implement support for unaccepted memory
> > > >
> > > > The UEFI v2.9 specification includes a new memory type to be used in
> > > > environments where the OS must accept memory that is provided from its
> > > > host. Before the introduction of this memory type, all memory was
> > > > accepted eagerly in the firmware. In order for the firmware to safely
> > > > stop accepting memory on the OS's behalf, the OS must affirmatively
> > > > indicate support to the firmware.
> > >
> > > I think it is a bad idea.
> > >
> > > This approach breaks use case with a bootloader between BIOS and OS.
> > > As the bootloader does ExitBootServices() it has to make the call on
> > > behalf of OS when it has no idea if the OS supports unaccepted.
> >
> > Nothing breaks, it'll error on the safe side.  If the protocol callback
> > is not called the firmware will simply accept all memory.  The guest OS
> > will only see unaccepted memory if it explicitly asked for it (assuming
> > the firmware wants know to support both cases, of course the firmware
> > could also enforce the one or the other and just not offer the
> > protocol).
>
> How bootloader suppose to know if OS will ask for unaccepted memory?
> It can't. It means the use-case with bootloader cannot ever use
> unaccepted memory. That's broken design.
>

I still don't understand why we need to support every imaginable
combination of firmware, bootloader and OS. Unaccepted memory only
exists on a special kind of virtual machine, which provides very
little added value unless you opt into the security and attestation
features, which are all heavily based on firmware protocols. So why
should care about a EFI-aware bootloader calling ExitBootServices()
and subsequently doing a legacy boot of Linux on such systems?

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Kirill A. Shutemov]

On Mon, Jan 16, 2023 at 02:11:26PM +0100, Ard Biesheuvel wrote:
> On Mon, 16 Jan 2023 at 13:31, Kirill A. Shutemov <kirill@shutemov.name> wrote:
> >
> > On Mon, Jan 16, 2023 at 11:56:48AM +0100, Gerd Hoffmann wrote:
> > > On Sat, Jan 14, 2023 at 01:20:24AM +0300, Kirill A. Shutemov wrote:
> > > > On Fri, Jan 13, 2023 at 09:29:26PM +0000, Dionna Glaze wrote:
> > > > > This patch depends on Kirill A. Shutemov's series
> > > > >
> > > > > [PATCHv8 00/14] mm, x86/cc: Implement support for unaccepted memory
> > > > >
> > > > > The UEFI v2.9 specification includes a new memory type to be used in
> > > > > environments where the OS must accept memory that is provided from its
> > > > > host. Before the introduction of this memory type, all memory was
> > > > > accepted eagerly in the firmware. In order for the firmware to safely
> > > > > stop accepting memory on the OS's behalf, the OS must affirmatively
> > > > > indicate support to the firmware.
> > > >
> > > > I think it is a bad idea.
> > > >
> > > > This approach breaks use case with a bootloader between BIOS and OS.
> > > > As the bootloader does ExitBootServices() it has to make the call on
> > > > behalf of OS when it has no idea if the OS supports unaccepted.
> > >
> > > Nothing breaks, it'll error on the safe side.  If the protocol callback
> > > is not called the firmware will simply accept all memory.  The guest OS
> > > will only see unaccepted memory if it explicitly asked for it (assuming
> > > the firmware wants know to support both cases, of course the firmware
> > > could also enforce the one or the other and just not offer the
> > > protocol).
> >
> > How bootloader suppose to know if OS will ask for unaccepted memory?
> > It can't. It means the use-case with bootloader cannot ever use
> > unaccepted memory. That's broken design.
> >
> 
> I still don't understand why we need to support every imaginable
> combination of firmware, bootloader and OS. Unaccepted memory only
> exists on a special kind of virtual machine, which provides very
> little added value unless you opt into the security and attestation
> features, which are all heavily based on firmware protocols. So why
> should care about a EFI-aware bootloader calling ExitBootServices()
> and subsequently doing a legacy boot of Linux on such systems?

Why break what works? Some users want it.

This patch adds complexity, breaks what works and the only upside will
turn into a dead weight soon.

There's alternative to add option to instruct firmware to accept all
memory from VMM side. It will serve legacy OS that doesn't know about
unaccepted memory and it is also can be use by latency-sensitive users
later on (analog of qemu -mem-prealloc).

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Dionna Glaze]

> > I still don't understand why we need to support every imaginable
> > combination of firmware, bootloader and OS. Unaccepted memory only
> > exists on a special kind of virtual machine, which provides very
> > little added value unless you opt into the security and attestation
> > features, which are all heavily based on firmware protocols. So why
> > should care about a EFI-aware bootloader calling ExitBootServices()
> > and subsequently doing a legacy boot of Linux on such systems?
>
> Why break what works? Some users want it.
>

The users that want legacy boot features will not be broken, they'll
only get a safe view of the memory map. I don't think it's right to
choose unsafe behavior for a legacy setup.

> This patch adds complexity, breaks what works and the only upside will
> turn into a dead weight soon.
>
> There's alternative to add option to instruct firmware to accept all
> memory from VMM side. It will serve legacy OS that doesn't know about
> unaccepted memory and it is also can be use by latency-sensitive users
> later on (analog of qemu -mem-prealloc).
>

This means that users of a distro that has not enabled unaccepted
memory support cannot simply start a VM with the usual command, but
instead have to know a baroque extra flag to get access to all the
memory that they configured the machine (and for a CSP customer, paid
for). That's not a good experience.

With GCE at least, you can't (shouldn't) associate the boot feature
flag with a disk image because disks are mutable. If a customer
upgrades their kernel after initially starting their VM, they can't
remove the flag due to the way image annotations work.

All of this headache goes away by adopting a small patch to the kernel
that calls a 0-ary protocol interface and keeping safe acceptance
behavior in the firmware. I think Gerd is right here that we should
treat it as a transition feature that we can remove later.

> --
>   Kiryl Shutsemau / Kirill A. Shutemov



-- 
-Dionna Glaze, PhD (she/her)

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Dave Hansen]

On 1/16/23 02:56, Gerd Hoffmann wrote:
>> And we add this protocol to address very temporary problem: once
>> unaccepted memory support get upstream it is just a dead weight.
> Maybe, maybe not.  unaccepted memory support has a Kconfig switch after
> all.  If we figure in 3-5 years that all distros have enabled it anyway
> we can drop it again.  For the transition period it will surely be
> useful.

I agree with Kirill here.

Having unaccepted memory *AND* this firmware-driven feature really is
just implementing the same thing twice.

I'd much rather have the Kconfig option forced on for all guests that
*might* need unaccepted memory support than carry redundant implementations.

Also, _if_ we allow folks to turn the Kconfig off and get access to all
their memory, they might get used to that.  Removing this firmware
interface from the kernel in a few years could be viewed as a
regression.  Then, we'll be stuck with this forever.

In any case, the firmware side of things didn't seem like _that_ much
code.  So, I'm not protesting *that* strongly.  But, I also don't
believe for a second that this is going to be removed in 3-5 years.

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Lendacky, Thomas]

On 1/16/23 15:22, Dave Hansen wrote:
> On 1/16/23 02:56, Gerd Hoffmann wrote:
>>> And we add this protocol to address very temporary problem: once
>>> unaccepted memory support get upstream it is just a dead weight.
>> Maybe, maybe not.  unaccepted memory support has a Kconfig switch after
>> all.  If we figure in 3-5 years that all distros have enabled it anyway
>> we can drop it again.  For the transition period it will surely be
>> useful.
> 
> I agree with Kirill here.
> 
> Having unaccepted memory *AND* this firmware-driven feature really is
> just implementing the same thing twice.

I'm not sure I follow you. This feature merely tells the firmware whether 
or not the OS supports unaccepted memory, which then tells the firmware 
whether it needs to accept the memory or whether the kernel can.

We have had SNP guest support since 5.19 without support for unaccepted 
memory. Imagine now using a newer OVMF that can support unaccepted memory. 
How does the firmware know whether it must accept all the memory or 
whether it can advertise unaccepted memory. By having the kernel call this 
boot service protocol once support for unaccepted memory is in place, the 
firmware now knows that it need not accept all the memory.

Thanks,
Tom

> 
> I'd much rather have the Kconfig option forced on for all guests that
> *might* need unaccepted memory support than carry redundant implementations.
> 
> Also, _if_ we allow folks to turn the Kconfig off and get access to all
> their memory, they might get used to that.  Removing this firmware
> interface from the kernel in a few years could be viewed as a
> regression.  Then, we'll be stuck with this forever.
> 
> In any case, the firmware side of things didn't seem like _that_ much
> code.  So, I'm not protesting *that* strongly.  But, I also don't
> believe for a second that this is going to be removed in 3-5 years.

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Kirill A. Shutemov]

On Mon, Jan 16, 2023 at 11:43:15AM -0800, Dionna Amalie Glaze wrote:
> > > I still don't understand why we need to support every imaginable
> > > combination of firmware, bootloader and OS. Unaccepted memory only
> > > exists on a special kind of virtual machine, which provides very
> > > little added value unless you opt into the security and attestation
> > > features, which are all heavily based on firmware protocols. So why
> > > should care about a EFI-aware bootloader calling ExitBootServices()
> > > and subsequently doing a legacy boot of Linux on such systems?
> >
> > Why break what works? Some users want it.
> >
> 
> The users that want legacy boot features will not be broken,

Why do you call boot with a bootloader a legacy feature?

> they'll only get a safe view of the memory map. I don't think it's right
> to choose unsafe behavior for a legacy setup.

Present memory map with unaccepted memory to OS that doesn't about it is
perfectly safe. This portion of the memory will be ignored. It is "feature
not [yet] implemented" case.

> > This patch adds complexity, breaks what works and the only upside will
> > turn into a dead weight soon.
> >
> > There's alternative to add option to instruct firmware to accept all
> > memory from VMM side. It will serve legacy OS that doesn't know about
> > unaccepted memory and it is also can be use by latency-sensitive users
> > later on (analog of qemu -mem-prealloc).
> >
> 
> This means that users of a distro that has not enabled unaccepted
> memory support cannot simply start a VM with the usual command, but
> instead have to know a baroque extra flag to get access to all the
> memory that they configured the machine (and for a CSP customer, paid
> for). That's not a good experience.

New features require enabling. It is not something new.

> With GCE at least, you can't (shouldn't) associate the boot feature
> flag with a disk image because disks are mutable. If a customer
> upgrades their kernel after initially starting their VM, they can't
> remove the flag due to the way image annotations work.

I guess a new VM has to be created, right? Doesn't sound like a big deal
to me.

The old will not break with upgraded kernel. Just not get benefit of the
feature.

> All of this headache goes away by adopting a small patch to the kernel
> that calls a 0-ary protocol interface and keeping safe acceptance
> behavior in the firmware. I think Gerd is right here that we should
> treat it as a transition feature that we can remove later.

Removing a feature is harder than adding one. How do you define that
"later" has come?

Anyway, I think we walk in a circle. I consider it a misfeature. If you
want still go this path, please add my

Nacked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Gerd Hoffmann]

On Tue, Jan 17, 2023 at 02:17:11AM +0300, Kirill A. Shutemov wrote:
> On Mon, Jan 16, 2023 at 11:43:15AM -0800, Dionna Amalie Glaze wrote:
> > > > I still don't understand why we need to support every imaginable
> > > > combination of firmware, bootloader and OS. Unaccepted memory only
> > > > exists on a special kind of virtual machine, which provides very
> > > > little added value unless you opt into the security and attestation
> > > > features, which are all heavily based on firmware protocols. So why
> > > > should care about a EFI-aware bootloader calling ExitBootServices()
> > > > and subsequently doing a legacy boot of Linux on such systems?
> > >
> > > Why break what works? Some users want it.
> > >
> > 
> > The users that want legacy boot features will not be broken,
> 
> Why do you call boot with a bootloader a legacy feature?

Linux efi kernels can be booted in two ways:

  (1) old/legacy: boot loader calls ExitBootServices and jumps to the
      kernel entry point.
  (2) new/efi stub: boot loader does *not* call ExitBootServices, but
      loads the linux kernel as efi binary instead.  The linux kernel
      efi stub calls ExitBootServices then.

All kernel version relevant here (new enough to support SEV-SNP / TDX)
have efi stub support, so (1) does not really matter in practice.

the efi stub was added *exactly* to handle cases like this one: the
kernel can do efi calls needed on its own without depending on the
boot loader doing it on behalf of the kernel.

> > This means that users of a distro that has not enabled unaccepted
> > memory support cannot simply start a VM with the usual command, but
> > instead have to know a baroque extra flag to get access to all the
> > memory that they configured the machine (and for a CSP customer, paid
> > for). That's not a good experience.
> 
> New features require enabling. It is not something new.

Asking user to manually configure something which can be handled
automatically just fine is a bad design.

take care,
  Gerd

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Gerd Hoffmann]

  Hi,

> In any case, the firmware side of things didn't seem like _that_ much
> code.  So, I'm not protesting *that* strongly.  But, I also don't
> believe for a second that this is going to be removed in 3-5 years.

If things are going roughly as I expect them to go (both tdx support and
unaccepted memory support land upstream this year; distros enable it by
default) we should be able to drop this when the 6.1-lts kernel goes
EOL.  First in edk2, later in linux too.

take care,
  Gerd

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Dionna Glaze]

>
> Why do you call boot with a bootloader a legacy feature?
>

Gerd answered this about EBS called from the bootloader.

> > they'll only get a safe view of the memory map. I don't think it's right
> > to choose unsafe behavior for a legacy setup.
>
> Present memory map with unaccepted memory to OS that doesn't about it is
> perfectly safe. This portion of the memory will be ignored. It is "feature
> not [yet] implemented" case.
>

SNP guest support is already in Linux, and it gets a full view of the
memory given to the VM. If the firmware ever introduces unaccepted
memory, then the kernel's behavior is retroactively broken without the
"accept all if AllowUnacceptedMemory() not called" behavior of the
UEFI.
The memory that existed before becomes ignored. This is not the right
approach IMO.

> > > This patch adds complexity, breaks what works and the only upside will
> > > turn into a dead weight soon.
> > >
> > > There's alternative to add option to instruct firmware to accept all
> > > memory from VMM side. It will serve legacy OS that doesn't know about
> > > unaccepted memory and it is also can be use by latency-sensitive users
> > > later on (analog of qemu -mem-prealloc).
> > >
> >
> > This means that users of a distro that has not enabled unaccepted
> > memory support cannot simply start a VM with the usual command, but
> > instead have to know a baroque extra flag to get access to all the
> > memory that they configured the machine (and for a CSP customer, paid
> > for). That's not a good experience.
>
> New features require enabling. It is not something new.
>

What I'm saying is that you're suggesting a feature _dis_abling
requirement, which is an antipattern. Any SNP user right now would
need to add a "don't use an unimplemented feature" flag to get access
to all its memory again.

> > With GCE at least, you can't (shouldn't) associate the boot feature
> > flag with a disk image because disks are mutable. If a customer
> > upgrades their kernel after initially starting their VM, they can't
> > remove the flag due to the way image annotations work.
>
> I guess a new VM has to be created, right? Doesn't sound like a big deal
> to me.
>

Usually it's not, but the retroactive need to create a new VM once the
firmware adds UEFI v2.9 support with unaccepted memory is a big deal.

> The old will not break with upgraded kernel. Just not get benefit of the
> feature.
>

A user buys access to a high memory VM: 768GiB. They then shut down
and bring it back up on a new firmware that uses unaccepted memory.

That VM goes from 785GiB free memory to 3GiB free memory at boot.

This is because all memory above 4GiB (and nothing there for the
3-4GiB MMIO hole) would be the unknown unaccepted memory type. We need
the accept-all-if-support-not-acked semantics with the protocol.

> > All of this headache goes away by adopting a small patch to the kernel
> > that calls a 0-ary protocol interface and keeping safe acceptance
> > behavior in the firmware. I think Gerd is right here that we should
> > treat it as a transition feature that we can remove later.
>
> Removing a feature is harder than adding one. How do you define that
> "later" has come?
>

Gerd's response of after 6.1-lts EOL is reasonable to me. At the same
time, both SEV-SNP and TDX's Kconfig would need to strictly require
unaccepted memory.

The semantics of the UEFI under the proposed protocol is allowed to
change the default behavior when the protocol is not exposed to the
OS. The default would then be to always introduce unaccepted memory
for TDX and SEV-SNP guests.

To Gerd's point, removing "first in edk2, later in linux too" I think
is backwards. We need all users of the protocol to agree that SEV-SNP
and TDX strictly imply unaccepted memory support. Only then can we
remove the protocol from EDK2.

> Anyway, I think we walk in a circle. I consider it a misfeature. If you
> want still go this path, please add my
>
> Nacked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
>

Thanks for your time discussing.


-- 
-Dionna Glaze, PhD (she/her)

Re: [edk2-devel] [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Gerd Hoffmann]

  Hi,

> To Gerd's point, removing "first in edk2, later in linux too" I think
> is backwards. We need all users of the protocol to agree that SEV-SNP
> and TDX strictly imply unaccepted memory support. Only then can we
> remove the protocol from EDK2.

Its not backwards.

edk2 dropping support first would result in break kernels without
support for unaccepted memory.  Which is why we wait until such
kernels are EOL.

Linux dropping support first would result in firmware accepting all
memory again.  So that isn't a good plan.  Thats why linux should
support the protocol a bit longer, while firmware versions which
expect negotiation happening are still in use.

take care,
  Gerd

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Ard Biesheuvel]

(cc'ing some folks whom I've discussed this with off-list today)

Full discussion here:
https://lore.kernel.org/linux-efi/20230113212926.2904735-1-dionnaglaze@google.com/

On Mon, 16 Jan 2023 at 23:46, Tom Lendacky <thomas.lendacky@amd.com> wrote:
>
> On 1/16/23 15:22, Dave Hansen wrote:
> > On 1/16/23 02:56, Gerd Hoffmann wrote:
> >>> And we add this protocol to address very temporary problem: once
> >>> unaccepted memory support get upstream it is just a dead weight.
> >> Maybe, maybe not.  unaccepted memory support has a Kconfig switch after
> >> all.  If we figure in 3-5 years that all distros have enabled it anyway
> >> we can drop it again.  For the transition period it will surely be
> >> useful.
> >
> > I agree with Kirill here.
> >
> > Having unaccepted memory *AND* this firmware-driven feature really is
> > just implementing the same thing twice.
>
> I'm not sure I follow you. This feature merely tells the firmware whether
> or not the OS supports unaccepted memory, which then tells the firmware
> whether it needs to accept the memory or whether the kernel can.
>
> We have had SNP guest support since 5.19 without support for unaccepted
> memory. Imagine now using a newer OVMF that can support unaccepted memory.
> How does the firmware know whether it must accept all the memory or
> whether it can advertise unaccepted memory. By having the kernel call this
> boot service protocol once support for unaccepted memory is in place, the
> firmware now knows that it need not accept all the memory.
>

So if people deploying SEV agree that this is a useful feature to
have, and people working on TDX saying this protocol must never exist,
I think the obvious conclusion is that OVMF should only expose it when
running on SEV.

However, I am still failing to grasp why there is disagreement here.
Linux already implements SEV support but unaccepted memory protocol is
not supported yet, and so it is crystal clear that we need something
to bridge the compatibility gap. Without this protocol, firmware must
never accept memory, and the OS must always take charge of this, even
if it prefers to accept memory eagerly.

With this protocol in place, acceptance becomes a policy decision,
where the default policy is 'accept' for OS implementations that have
no understanding of unaccepted memory or the protocol. 'Enlightened'
OSes can still decide not to call the protocol and therefore not
having to bother with acceptance at all, given that the firmware will
take care of it.

As for the 'legacy' boot method: this bootloader can decide for itself
whether or not it needs to invoke the protocol, and can invent its own
methods for communicating/inspecting the OS image to obtain the
information to base this decision on. This is outside of the scope of
EFI. However, I also disagree with the binary 'no solution shall
exist' vs 'a solution must cover every imaginable combination of
bootloader and OS image': it makes sense to be pragmatic here, and
limit ourselves to what people are actually deploying. And given the
default behavior fo 'accept everything', the only penalty for ignoring
the legacy bootloader case is a slower boot.

I have been on the sidelines for most of the OVMF and Linux
development regarding confidential compute, but where I did get
involved, it was to try and reach consensus between the different
technologies (including the ARM one), to avoid ending up with radical
different approaches for doing the same thing.

However, I guess we're at a point where SEV and TDX really want
different solutions, so I think divergence might be the way to
proceed.

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Dave Hansen]

On 1/18/23 07:09, Ard Biesheuvel wrote:
> However, I guess we're at a point where SEV and TDX really want
> different solutions, so I think divergence might be the way to
> proceed.

I don't think they want different things really.

TDX doesn't need this protocol.  It sounds like SEV does need it,
though.  That doesn't mean they really diverge.  They're *both* going to
have to poke at this protocol knob to get the firmware to not accept the
memory.

This does slightly change the motivation for doing explicit unaccepted
memory support in the kernel.

I also don't know _quite_ how this will look to a guest.  For instance,
will they see different memory maps based on which protocol they are
using?  I assume so, but didn't see any of that explicitly mentioned in
this patch.

Re: [PATCH v2] x86/efi: Safely enable unaccepted memory in UEFI

[Ard Biesheuvel]

On Wed, 18 Jan 2023 at 16:41, Dave Hansen <dave.hansen@intel.com> wrote:
>
> On 1/18/23 07:09, Ard Biesheuvel wrote:
> > However, I guess we're at a point where SEV and TDX really want
> > different solutions, so I think divergence might be the way to
> > proceed.
>
> I don't think they want different things really.
>
> TDX doesn't need this protocol.  It sounds like SEV does need it,
> though.  That doesn't mean they really diverge.  They're *both* going to
> have to poke at this protocol knob to get the firmware to not accept the
> memory.
>

No, on TDX, the firmware would never accept all memory. On SEV, it
would only do so if the protocol has not been called prior to the call
to ExitBootServices().

> This does slightly change the motivation for doing explicit unaccepted
> memory support in the kernel.
>

Not on TDX.

> I also don't know _quite_ how this will look to a guest.  For instance,
> will they see different memory maps based on which protocol they are
> using?  I assume so, but didn't see any of that explicitly mentioned in
> this patch.

The EFI memory map will not contain ranges of type
EFI_UNACCEPTED_MEMORY if the memory was accepted on behalf of the OS
by the firmware. That is the point, really, as non-enlightened OSes
will ignore those.