On 16.01.24 г. 9:28 ч., Kirill A. Shutemov wrote:
<snip>
@@ -41,6 +44,9 @@
static atomic_long_t nr_shared;
+static atomic_t conversions_in_progress;
+static bool conversion_allowed = true;
Given the usage model of this variable, shouldn't it be simply accessed via
READ/WRITE_ONCE macros?
What do you see it changing?
Serving as documentation that you are accessing a shared variable
without an explicit lock (unless I'm missing something).
conversion_allowed can be read by multiple threads, no ? And it's
written by a single thread?
<snip>
+static void tdx_kexec_stop_conversion(bool crash)
+{
+ /* Stop new private<->shared conversions */
+ conversion_allowed = false;
What's the logic behind this compiler barrier?
Disallow compiler to push the assignment past atomic_read() loop below.
Not sure if anything else prevents such reorder without the barrier.
And I don't think WRITE_ONCE() will do the trick. It only prevents
multiple writes, but doesn't prevent reorders agains accesses
non-READ_ONCE()/WRITE_ONCE() accesses.
+ barrier();
+
+ /*
+ * 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");
+}
+
<snip>
diff --git a/arch/x86/include/asm/x86_init.h b/arch/x86/include/asm/x86_init.h
index c9503fe2d13a..3196ff20a29e 100644
--- a/arch/x86/include/asm/x86_init.h
+++ b/arch/x86/include/asm/x86_init.h
@@ -154,6 +154,8 @@ struct x86_guest {
int (*enc_status_change_finish)(unsigned long vaddr, int npages, bool
enc);
bool (*enc_tlb_flush_required)(bool enc);
bool (*enc_cache_flush_required)(void);
+ void (*enc_kexec_stop_conversion)(bool crash);
+ void (*enc_kexec_unshare_mem)(void);
These are only being initialized in the TDX case, but called in all cases
when CC_ATTR_GUEST_MEM_ENCRYPT is true, which includes AMD. So it would
cause a crash, no ? Shouldn't you also introduce noop handlers initialized
in the default x86_platform struct in arch/x86/kernel/x86_init.c ?
kexec on AMD will not work without them, I think. But noops makes sense
anyway. Will fix.
I'm not disputing whether those are needed for AMD or not, that way I
see it you make those callbacks mandatory in the case of
CC_ATTR_GUEST_MEM_ENCRYPT being present, yet only implement them for
TDX. So in the case of AMD they will be NULL and so AMD with kexec
enabled (albeit erroneously) will crash, no ?
_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
