[PATCHv5,10/16] x86/tdx: Convert shared memory back to private on kexec
Commit Message
TDX guests allocate shared buffers to perform I/O. It is done by
allocating pages normally from the buddy allocator and converting them
to shared with set_memory_decrypted().
The second kernel has no idea what memory is converted this way. It only
sees E820_TYPE_RAM.
Accessing shared memory via private mapping is fatal. It leads to
unrecoverable TD exit.
On kexec walk direct mapping and convert all shared memory back to
private. It makes all RAM private again and second kernel may use it
normally.
The conversion occurs in two steps: stopping new conversions and
unsharing all memory. In the case of normal kexec, the stopping of
conversions takes place while scheduling is still functioning. This
allows for waiting until any ongoing conversions are finished. The
second step is carried out when all CPUs except one are inactive and
interrupts are disabled. This prevents any conflicts with code that may
access shared memory.
Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
---
arch/x86/coco/tdx/tdx.c | 119 +++++++++++++++++++++++++++++++-
arch/x86/include/asm/x86_init.h | 2 +
arch/x86/kernel/crash.c | 6 ++
arch/x86/kernel/reboot.c | 13 ++++
4 files changed, 138 insertions(+), 2 deletions(-)
Comments
The break apart looks good.
On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> + while (addr < end) {
> + unsigned long size;
> + unsigned int level;
> + pte_t *pte;
> +
> + pte = lookup_address(addr, &level);
> + size = page_level_size(level);
> +
> + if (pte && pte_decrypted(*pte)) {
> + int pages = size / PAGE_SIZE;
> +
> + /*
> + * Touching memory with shared bit set
> triggers implicit
> + * conversion to shared.
> + *
> + * Make sure nobody touches the shared range
> from
> + * now on.
> + */
> + set_pte(pte, __pte(0));
Does this need a full shootdown at this point, at least in the crash
case? A local flush for the non-crash case might be nice too.
Also, probably a barrier() between set_pte() and
tdx_enc_status_changed().
> +
> + if (!tdx_enc_status_changed(addr, pages,
> true)) {
> + pr_err("Failed to unshare range %#lx-
> %#lx\n",
> + addr, addr + size);
> + }
On Fri, Jan 05, 2024 at 07:38:48PM +0000, Edgecombe, Rick P wrote:
> The break apart looks good.
>
> On Sat, 2023-12-23 at 02:52 +0300, Kirill A. Shutemov wrote:
> > + while (addr < end) {
> > + unsigned long size;
> > + unsigned int level;
> > + pte_t *pte;
> > +
> > + pte = lookup_address(addr, &level);
> > + size = page_level_size(level);
> > +
> > + if (pte && pte_decrypted(*pte)) {
> > + int pages = size / PAGE_SIZE;
> > +
> > + /*
> > + * Touching memory with shared bit set
> > triggers implicit
> > + * conversion to shared.
> > + *
> > + * Make sure nobody touches the shared range
> > from
> > + * now on.
> > + */
> > + set_pte(pte, __pte(0));
>
> Does this need a full shootdown at this point, at least in the crash
> case? A local flush for the non-crash case might be nice too.
See __flush_tlb_all() after the loop.
> Also, probably a barrier() between set_pte() and
> tdx_enc_status_changed().
But why? There's no concurrency at this point. Interrupts are disabled and
only one CPU is active. Nobody can touch the memory relevant for the PTE.
On Sat, 2024-01-06 at 03:59 +0300, kirill.shutemov@linux.intel.com
wrote:
> But why? There's no concurrency at this point. Interrupts are
> disabled and
> only one CPU is active. Nobody can touch the memory relevant for the
> PTE.
Oh, right, sorry. I had thought there could be other CPUs active at
that point.
On 23.12.23 г. 1:52 ч., Kirill A. Shutemov wrote:
> TDX guests allocate shared buffers to perform I/O. It is done by
> allocating pages normally from the buddy allocator and converting them
> to shared with set_memory_decrypted().
>
> The second kernel has no idea what memory is converted this way. It only
> sees E820_TYPE_RAM.
>
> Accessing shared memory via private mapping is fatal. It leads to
> unrecoverable TD exit.
>
> On kexec walk direct mapping and convert all shared memory back to
> private. It makes all RAM private again and second kernel may use it
> normally.
>
> The conversion occurs in two steps: stopping new conversions and
> unsharing all memory. In the case of normal kexec, the stopping of
> conversions takes place while scheduling is still functioning. This
> allows for waiting until any ongoing conversions are finished. The
> second step is carried out when all CPUs except one are inactive and
> interrupts are disabled. This prevents any conflicts with code that may
> access shared memory.
>
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> ---
> arch/x86/coco/tdx/tdx.c | 119 +++++++++++++++++++++++++++++++-
> arch/x86/include/asm/x86_init.h | 2 +
> arch/x86/kernel/crash.c | 6 ++
> arch/x86/kernel/reboot.c | 13 ++++
> 4 files changed, 138 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> index 8a49484a2917..5c64db168edd 100644
> --- a/arch/x86/coco/tdx/tdx.c
> +++ b/arch/x86/coco/tdx/tdx.c
> @@ -6,8 +6,10 @@
>
> #include <linux/cpufeature.h>
> #include <linux/debugfs.h>
> +#include <linux/delay.h>
> #include <linux/export.h>
> #include <linux/io.h>
> +#include <linux/kexec.h>
> #include <asm/coco.h>
> #include <asm/tdx.h>
> #include <asm/vmx.h>
> @@ -15,6 +17,7 @@
> #include <asm/insn.h>
> #include <asm/insn-eval.h>
> #include <asm/pgtable.h>
> +#include <asm/set_memory.h>
>
> /* MMIO direction */
> #define EPT_READ 0
> @@ -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?
> +
> static inline bool pte_decrypted(pte_t pte)
> {
> return cc_mkdec(pte_val(pte)) == pte_val(pte);
> @@ -726,6 +732,14 @@ static bool tdx_tlb_flush_required(bool private)
>
> static bool tdx_cache_flush_required(void)
> {
> + /*
> + * Avoid issuing CLFLUSH on set_memory_decrypted() if conversions
> + * stopped. Otherwise it can race with unshare_all_memory() and trigger
> + * implicit conversion to shared.
> + */
> + if (!conversion_allowed)
> + return false;
> +
> /*
> * AMD SME/SEV can avoid cache flushing if HW enforces cache coherence.
> * TDX doesn't have such capability.
> @@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> bool enc)
> {
> + atomic_inc(&conversions_in_progress);
> +
> + /*
> + * Check after bumping conversions_in_progress to serialize
> + * against tdx_shutdown().
> + */
> + if (!conversion_allowed) {
> + atomic_dec(&conversions_in_progress);
> + return -EBUSY;
> + }
nit: Can you make the inc of conversions_in_progress be done here, this
eliminated the atomic_dec in case they aren't. Somewhat simplifies the
logic.
> +
> /*
> * Only handle shared->private conversion here.
> * See the comment in tdx_early_init().
> */
> - if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> + if (enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> + atomic_dec(&conversions_in_progress);
> return -EIO;
> + }
>
> return 0;
> }
> @@ -826,17 +853,102 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> * Only handle private->shared conversion here.
> * See the comment in tdx_early_init().
> */
> - if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> + if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> + atomic_dec(&conversions_in_progress);
> return -EIO;
> + }
>
> if (enc)
> atomic_long_sub(numpages, &nr_shared);
> else
> atomic_long_add(numpages, &nr_shared);
>
> + atomic_dec(&conversions_in_progress);
> +
> return 0;
> }
>
> +static void tdx_kexec_stop_conversion(bool crash)
> +{
> + /* Stop new private<->shared conversions */
> + conversion_allowed = false;
What's the logic behind this compiler barrier?
> + 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 ?
> };
>
> /**
> diff --git a/arch/x86/kernel/crash.c b/arch/x86/kernel/crash.c
> index c92d88680dbf..b99bd28ad22f 100644
> --- a/arch/x86/kernel/crash.c
> +++ b/arch/x86/kernel/crash.c
> @@ -40,6 +40,7 @@
> #include <asm/intel_pt.h>
> #include <asm/crash.h>
> #include <asm/cmdline.h>
> +#include <asm/tdx.h>
>
> /* Used while preparing memory map entries for second kernel */
> struct crash_memmap_data {
> @@ -107,6 +108,11 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
>
> crash_smp_send_stop();
>
> + if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
> + x86_platform.guest.enc_kexec_stop_conversion(true);
> + x86_platform.guest.enc_kexec_unshare_mem();
> + }
> +
> cpu_emergency_disable_virtualization();
>
> /*
> diff --git a/arch/x86/kernel/reboot.c b/arch/x86/kernel/reboot.c
> index 830425e6d38e..16dde83df49a 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_stop_conversion() while all CPUs are still active and
> + * interrupts are enabled. This will allow all in-flight memory
> + * conversions to finish cleanly.
> + */
> + if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
> + x86_platform.guest.enc_kexec_stop_conversion(false);
> +
> /* Stop the cpus and apics */
> #ifdef CONFIG_X86_IO_APIC
> /*
> @@ -752,6 +762,9 @@ void native_machine_shutdown(void)
> #ifdef CONFIG_X86_64
> x86_platform.iommu_shutdown();
> #endif
> +
> + if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
> + x86_platform.guest.enc_kexec_unshare_mem();
> }
>
> static void __machine_emergency_restart(int emergency)
On Mon, Jan 15, 2024 at 12:53:42PM +0200, Nikolay Borisov wrote:
>
>
> On 23.12.23 г. 1:52 ч., Kirill A. Shutemov wrote:
> > TDX guests allocate shared buffers to perform I/O. It is done by
> > allocating pages normally from the buddy allocator and converting them
> > to shared with set_memory_decrypted().
> >
> > The second kernel has no idea what memory is converted this way. It only
> > sees E820_TYPE_RAM.
> >
> > Accessing shared memory via private mapping is fatal. It leads to
> > unrecoverable TD exit.
> >
> > On kexec walk direct mapping and convert all shared memory back to
> > private. It makes all RAM private again and second kernel may use it
> > normally.
> >
> > The conversion occurs in two steps: stopping new conversions and
> > unsharing all memory. In the case of normal kexec, the stopping of
> > conversions takes place while scheduling is still functioning. This
> > allows for waiting until any ongoing conversions are finished. The
> > second step is carried out when all CPUs except one are inactive and
> > interrupts are disabled. This prevents any conflicts with code that may
> > access shared memory.
> >
> > Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> > Reviewed-by: Rick Edgecombe <rick.p.edgecombe@intel.com>
> > ---
> > arch/x86/coco/tdx/tdx.c | 119 +++++++++++++++++++++++++++++++-
> > arch/x86/include/asm/x86_init.h | 2 +
> > arch/x86/kernel/crash.c | 6 ++
> > arch/x86/kernel/reboot.c | 13 ++++
> > 4 files changed, 138 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c
> > index 8a49484a2917..5c64db168edd 100644
> > --- a/arch/x86/coco/tdx/tdx.c
> > +++ b/arch/x86/coco/tdx/tdx.c
> > @@ -6,8 +6,10 @@
> > #include <linux/cpufeature.h>
> > #include <linux/debugfs.h>
> > +#include <linux/delay.h>
> > #include <linux/export.h>
> > #include <linux/io.h>
> > +#include <linux/kexec.h>
> > #include <asm/coco.h>
> > #include <asm/tdx.h>
> > #include <asm/vmx.h>
> > @@ -15,6 +17,7 @@
> > #include <asm/insn.h>
> > #include <asm/insn-eval.h>
> > #include <asm/pgtable.h>
> > +#include <asm/set_memory.h>
> > /* MMIO direction */
> > #define EPT_READ 0
> > @@ -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?
> > +
> > static inline bool pte_decrypted(pte_t pte)
> > {
> > return cc_mkdec(pte_val(pte)) == pte_val(pte);
> > @@ -726,6 +732,14 @@ static bool tdx_tlb_flush_required(bool private)
> > static bool tdx_cache_flush_required(void)
> > {
> > + /*
> > + * Avoid issuing CLFLUSH on set_memory_decrypted() if conversions
> > + * stopped. Otherwise it can race with unshare_all_memory() and trigger
> > + * implicit conversion to shared.
> > + */
> > + if (!conversion_allowed)
> > + return false;
> > +
> > /*
> > * AMD SME/SEV can avoid cache flushing if HW enforces cache coherence.
> > * TDX doesn't have such capability.
> > @@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> > static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> > bool enc)
> > {
> > + atomic_inc(&conversions_in_progress);
> > +
> > + /*
> > + * Check after bumping conversions_in_progress to serialize
> > + * against tdx_shutdown().
> > + */
> > + if (!conversion_allowed) {
> > + atomic_dec(&conversions_in_progress);
> > + return -EBUSY;
> > + }
>
> nit: Can you make the inc of conversions_in_progress be done here, this
> eliminated the atomic_dec in case they aren't. Somewhat simplifies the
> logic.
Okay, fair enough. Will change.
> > +
> > /*
> > * Only handle shared->private conversion here.
> > * See the comment in tdx_early_init().
> > */
> > - if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> > + if (enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> > + atomic_dec(&conversions_in_progress);
> > return -EIO;
> > + }
> > return 0;
> > }
> > @@ -826,17 +853,102 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
> > * Only handle private->shared conversion here.
> > * See the comment in tdx_early_init().
> > */
> > - if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
> > + if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
> > + atomic_dec(&conversions_in_progress);
> > return -EIO;
> > + }
> > if (enc)
> > atomic_long_sub(numpages, &nr_shared);
> > else
> > atomic_long_add(numpages, &nr_shared);
> > + atomic_dec(&conversions_in_progress);
> > +
> > return 0;
> > }
> > +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.
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 ?
>
On Tue, Jan 16, 2024 at 10:01:47AM +0200, Nikolay Borisov wrote:
>
>
> 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?
I don't think READ_ONCE()/WRITE_ONCE() have documentation sense you imply.
I would argue adding them will add more confusion as they serve no purpose
in this context: issuing multiple loads/stores for the variable have no
impact on outcome.
> > 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 ?
As I said, I will fix it.
On Tue, Jan 16, 2024 at 10:28:22AM +0300, Kirill A. Shutemov wrote:
> > > @@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
> > > static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
> > > bool enc)
> > > {
> > > + atomic_inc(&conversions_in_progress);
> > > +
> > > + /*
> > > + * Check after bumping conversions_in_progress to serialize
> > > + * against tdx_shutdown().
s/tdx_shutdown/tdx_kexec_stop_conversion/
> > > + */
> > > + if (!conversion_allowed) {
> > > + atomic_dec(&conversions_in_progress);
> > > + return -EBUSY;
> > > + }
> >
> > nit: Can you make the inc of conversions_in_progress be done here, this
> > eliminated the atomic_dec in case they aren't. Somewhat simplifies the
> > logic.
>
> Okay, fair enough. Will change.
Actually, no, it will breaks serialization.
Consider following scenario (includes change you proposed):
CPU0 CPU1
tdx_enc_status_change_prepare()
if (!conversion_allowed) // false
tdx_kexec_stop_conversion()
conversion_allowed = false;
<...>
<return as no conversion in flight>
<continue with conversion>
Incrementing conversion_in_progress before checking conversion_allowed
will make tdx_kexec_stop_conversion() to spin until CPU0 is done with
conversion.
@@ -6,8 +6,10 @@
#include <linux/cpufeature.h>
#include <linux/debugfs.h>
+#include <linux/delay.h>
#include <linux/export.h>
#include <linux/io.h>
+#include <linux/kexec.h>
#include <asm/coco.h>
#include <asm/tdx.h>
#include <asm/vmx.h>
@@ -15,6 +17,7 @@
#include <asm/insn.h>
#include <asm/insn-eval.h>
#include <asm/pgtable.h>
+#include <asm/set_memory.h>
/* MMIO direction */
#define EPT_READ 0
@@ -41,6 +44,9 @@
static atomic_long_t nr_shared;
+static atomic_t conversions_in_progress;
+static bool conversion_allowed = true;
+
static inline bool pte_decrypted(pte_t pte)
{
return cc_mkdec(pte_val(pte)) == pte_val(pte);
@@ -726,6 +732,14 @@ static bool tdx_tlb_flush_required(bool private)
static bool tdx_cache_flush_required(void)
{
+ /*
+ * Avoid issuing CLFLUSH on set_memory_decrypted() if conversions
+ * stopped. Otherwise it can race with unshare_all_memory() and trigger
+ * implicit conversion to shared.
+ */
+ if (!conversion_allowed)
+ return false;
+
/*
* AMD SME/SEV can avoid cache flushing if HW enforces cache coherence.
* TDX doesn't have such capability.
@@ -809,12 +823,25 @@ static bool tdx_enc_status_changed(unsigned long vaddr, int numpages, bool enc)
static int tdx_enc_status_change_prepare(unsigned long vaddr, int numpages,
bool enc)
{
+ atomic_inc(&conversions_in_progress);
+
+ /*
+ * Check after bumping conversions_in_progress to serialize
+ * against tdx_shutdown().
+ */
+ if (!conversion_allowed) {
+ atomic_dec(&conversions_in_progress);
+ return -EBUSY;
+ }
+
/*
* Only handle shared->private conversion here.
* See the comment in tdx_early_init().
*/
- if (enc && !tdx_enc_status_changed(vaddr, numpages, enc))
+ if (enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
+ atomic_dec(&conversions_in_progress);
return -EIO;
+ }
return 0;
}
@@ -826,17 +853,102 @@ static int tdx_enc_status_change_finish(unsigned long vaddr, int numpages,
* Only handle private->shared conversion here.
* See the comment in tdx_early_init().
*/
- if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc))
+ if (!enc && !tdx_enc_status_changed(vaddr, numpages, enc)) {
+ atomic_dec(&conversions_in_progress);
return -EIO;
+ }
if (enc)
atomic_long_sub(numpages, &nr_shared);
else
atomic_long_add(numpages, &nr_shared);
+ atomic_dec(&conversions_in_progress);
+
return 0;
}
+static void tdx_kexec_stop_conversion(bool crash)
+{
+ /* Stop new private<->shared conversions */
+ conversion_allowed = false;
+ 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");
+}
+
+static void tdx_kexec_unshare_mem(void)
+{
+ unsigned long addr, end;
+ long found = 0, shared;
+
+ /*
+ * Walk direct mapping and convert all shared memory back to private,
+ */
+
+ addr = PAGE_OFFSET;
+ end = PAGE_OFFSET + get_max_mapped();
+
+ while (addr < end) {
+ unsigned long size;
+ unsigned int level;
+ pte_t *pte;
+
+ pte = lookup_address(addr, &level);
+ size = page_level_size(level);
+
+ if (pte && pte_decrypted(*pte)) {
+ int pages = size / PAGE_SIZE;
+
+ /*
+ * Touching memory with shared bit set triggers implicit
+ * conversion to shared.
+ *
+ * Make sure nobody touches the shared range from
+ * now on.
+ */
+ set_pte(pte, __pte(0));
+
+ if (!tdx_enc_status_changed(addr, pages, true)) {
+ pr_err("Failed to unshare range %#lx-%#lx\n",
+ addr, addr + size);
+ }
+
+ found += pages;
+ }
+
+ addr += size;
+ }
+
+ __flush_tlb_all();
+
+ shared = atomic_long_read(&nr_shared);
+ if (shared != found) {
+ pr_err("shared page accounting is off\n");
+ pr_err("nr_shared = %ld, nr_found = %ld\n", shared, found);
+ }
+}
+
void __init tdx_early_init(void)
{
struct tdx_module_args args = {
@@ -896,6 +1008,9 @@ void __init tdx_early_init(void)
x86_platform.guest.enc_cache_flush_required = tdx_cache_flush_required;
x86_platform.guest.enc_tlb_flush_required = tdx_tlb_flush_required;
+ x86_platform.guest.enc_kexec_stop_conversion = tdx_kexec_stop_conversion;
+ x86_platform.guest.enc_kexec_unshare_mem = tdx_kexec_unshare_mem;
+
/*
* TDX intercepts the RDMSR to read the X2APIC ID in the parallel
* bringup low level code. That raises #VE which cannot be handled
@@ -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);
};
/**
@@ -40,6 +40,7 @@
#include <asm/intel_pt.h>
#include <asm/crash.h>
#include <asm/cmdline.h>
+#include <asm/tdx.h>
/* Used while preparing memory map entries for second kernel */
struct crash_memmap_data {
@@ -107,6 +108,11 @@ void native_machine_crash_shutdown(struct pt_regs *regs)
crash_smp_send_stop();
+ if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT)) {
+ x86_platform.guest.enc_kexec_stop_conversion(true);
+ x86_platform.guest.enc_kexec_unshare_mem();
+ }
+
cpu_emergency_disable_virtualization();
/*
@@ -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_stop_conversion() while all CPUs are still active and
+ * interrupts are enabled. This will allow all in-flight memory
+ * conversions to finish cleanly.
+ */
+ if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
+ x86_platform.guest.enc_kexec_stop_conversion(false);
+
/* Stop the cpus and apics */
#ifdef CONFIG_X86_IO_APIC
/*
@@ -752,6 +762,9 @@ void native_machine_shutdown(void)
#ifdef CONFIG_X86_64
x86_platform.iommu_shutdown();
#endif
+
+ if (cc_platform_has(CC_ATTR_GUEST_MEM_ENCRYPT) && kexec_in_progress)
+ x86_platform.guest.enc_kexec_unshare_mem();
}
static void __machine_emergency_restart(int emergency)