Re: [PATCH v11 18/20] x86: Handle TDX erratum to reset TDX private memory during kexec() and reboot

[Huang, Kai]

On Mon, 2023-06-19 at 16:41 -0700, Dave Hansen wrote:
> On 6/19/23 07:46, kirill.shutemov@xxxxxxxxxxxxxxx wrote:
> > > > 
> > > > Using atomic_set() requires changing tdmr->pamt_4k_base to atomic_t, which is a
> > > > little bit silly or overkill IMHO.  Looking at the code, it seems
> > > > arch_atomic_set() simply uses __WRITE_ONCE():
> > > How about _adding_ a variable that protects tdmr->pamt_4k_base?
> > > Wouldn't that be more straightforward than mucking around with existing
> > > types?
> > What's wrong with simple global spinlock that protects all tdmr->pamt_*?
> > It is much easier to follow than a custom serialization scheme.
> 
> Quick, what prevents a:
> 
> 	spin_lock() => #MC => spin_lock()
> 
> deadlock?
> 
> Plain old test/sets don't deadlock ever.

Agreed.  So I think having any locking in #MC handle is kinda dangerous. 

Adding "a" variable has another advantage:  We can have a more precise result of
whether we need to reset PAMT pages, even those PAMTs are already allocated and
set to the TDMRs, because the TDX module only starts to write PAMTs using global
KeyID until some SEAMCALL.

Any comments to below?

+static bool tdx_private_mem_begin;
+
 /*
  * Wrapper of __seamcall() to convert SEAMCALL leaf function error code
  * to kernel error code.  @seamcall_ret and @out contain the SEAMCALL
@@ -1141,6 +1143,8 @@ static int init_tdx_module(void)
         */
        wbinvd_on_all_cpus();
 
+       WRITE_ONCE(tdx_private_mem_begin, true);
+
        /* Config the key of global KeyID on all packages */
        ret = config_global_keyid();
        if (ret)
@@ -1463,6 +1467,14 @@ static void tdx_memory_shutdown(void)
         */
        WARN_ON_ONCE(num_online_cpus() != 1);
 
+       /*
+        * It's not possible to have any TDX private pages if the TDX
+        * module hasn't started to write any memory using the global
+        * KeyID.
+        */
+       if (!READ_ONCE(tdx_private_mem_begin))
+               return;
+
        tdmrs_reset_pamt_all(&tdx_tdmr_list);