Re: [PATCHv4 10/14] x86/tdx: Convert shared memory back to private on kexec

On Wed, Dec 06, 2023 at 01:28:08AM +0000, Edgecombe, Rick P wrote:

On Tue, 2023-12-05 at 03:45 +0300, Kirill A. Shutemov wrote: 

quoted

+static void tdx_kexec_unshare_mem(bool crash)
+{
+       unsigned long addr, end;
+       long found = 0, shared;
+
+       /* Stop new private<->shared conversions */
+       conversion_allowed = false;

I wonder if this might need a compiler barrier here to be totally safe.
I'm not sure.

Yeah, it should be cleaner with a barrier.

quoted

+
+       /*
+        * Crash kernel reaches here with interrupts disabled: can't
wait for
+        * conversions to finish.
+        *
+        * If race happened, just report and proceed.
+        */
+       if (!crash) {
+               unsigned long timeout;
+
+               /*
+                * Wait for in-flight conversions to complete.
+                *
+                * Do not wait more than 30 seconds.
+                */
+               timeout = 30 * USEC_PER_SEC;
+               while (atomic_read(&conversions_in_progress) &&
timeout--)
+                       udelay(1);
+       }
+
+       if (atomic_read(&conversions_in_progress))
+               pr_warn("Failed to finish shared<->private
conversions\n");

I can't think of any non-ridiculous way to handle this case. Maybe we
need VMM help.

Do you see a specific way how VMM can help here?

quoted

diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
index 830425e6d38e..c81afffaa954 100644
--- a/arch/x86/kernel/reboot.c
+++ b/arch/x86/kernel/reboot.c

@@ -12,6 +12,7 @@

 #include <linux/delay.h>
 #include <linux/objtool.h>
 #include <linux/pgtable.h>
+#include <linux/kexec.h>
 #include <acpi/reboot.h>
 #include <asm/io.h>
 #include <asm/apic.h>

@@ -31,6 +32,7 @@

 #include <asm/realmode.h>
 #include <asm/x86_init.h>
 #include <asm/efi.h>
+#include <asm/tdx.h>
 
 /*
  * Power off function, if any

@@ -716,6 +718,14 @@ static void

native_machine_emergency_restart(void)
 
 void native_machine_shutdown(void)
 {
+       /*
+        * Call enc_kexec_unshare_mem() while all CPUs are still
active and
+        * interrupts are enabled. This will allow all in-flight
memory
+        * conversions to finish cleanly before unsharing all memory.
+        */
+       if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) &&
kexec_in_progress)
+               x86_platform.guest.enc_kexec_unshare_mem(false);

These questions are coming from an incomplete understanding of the
kexec/reboot operation. Please disregard if it is not helpful.

By doing this while other tasks can still run, it handles the
conversion races in the !crash case. But then it sets shared pages to
NP. What happens if another active task tries to write to one?

I guess we rely on the kernel_restart_prepare()->device_shutdown() to
clean up, which runs before native_machine_shutdown(). So there might
be conversions in progress when tdx_kexec_unshare_mem() is called, from
the allocator work queues. But the actual memory won't be accessed
during that operation.

Right, devices has to be shutdown by then.

But the console must be active? Or otherwise who can see these
warnings. It doesn't use a shared page? Or the KVM clock, which looks
to clean up at cpu tear down, which now happens after
tdx_kexec_unshare_mem()? So I wonder if there might be cases.

Virtio console is not functional by then, but serial is. Serial uses port
I/O and doesn't need shared memory.

If so, maybe you could halt the conversions in
native_machine_shutdown(), then do the actual reset to private after
tasks can't schedule.

It would also mean that we cannot use set_memory_np() there as it requires
sleepable context. I would rather keep conversion in
native_machine_shutdown() path.

I'd still wonder about if anything might try to
access a shared page triggered by the console output.

set_memory_np() would make it obvious if it ever happens.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov