Received-SPF: pass (google.com: domain of linux-kernel+bounces-27276-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Tue, 16 Jan 2024 13:53:24 +0300
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
Message-ID: <20240116105324.5kz6b2b6cbv6g4xn@box.shutemov.name>
References: <20231222235209.32143-1-kirill.shutemov@linux.intel.com>
 <20231222235209.32143-11-kirill.shutemov@linux.intel.com>
 <89e8722b-661b-4319-8018-06705b366c62@suse.com>
 <20240116072822.pvzseyqry56eqa4j@box.shutemov.name>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
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