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

From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com>
To: Nikolay Borisov <nik.borisov@suse.com>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
	Dave Hansen <dave.hansen@linux.intel.com>,
	x86@kernel.org, "Rafael J. Wysocki" <rafael@kernel.org>,
	Peter Zijlstra <peterz@infradead.org>,
	Adrian Hunter <adrian.hunter@intel.com>,
	Kuppuswamy Sathyanarayanan
	<sathyanarayanan.kuppuswamy@linux.intel.com>,
	Elena Reshetova <elena.reshetova@intel.com>,
	Jun Nakajima <jun.nakajima@intel.com>,
	Rick Edgecombe <rick.p.edgecombe@intel.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	"Kalra, Ashish" <ashish.kalra@amd.com>,
	Sean Christopherson <seanjc@google.com>,
	"Huang, Kai" <kai.huang@intel.com>, Baoquan He <bhe@redhat.com>,
	kexec@lists.infradead.org, linux-coco@lists.linux.dev,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCHv5 10/16] x86/tdx: Convert shared memory back to private on kexec
Date: Tue, 16 Jan 2024 13:53:24 +0300	[thread overview]
Message-ID: <20240116105324.5kz6b2b6cbv6g4xn@box.shutemov.name> (raw)
In-Reply-To: <20240116072822.pvzseyqry56eqa4j@box.shutemov.name>

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.

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec