Received-SPF: pass (google.com: domain of linux-kernel+bounces-27108-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <bad09d97-d99b-4231-a481-c14ed0f8d59d@suse.com>
Date: Tue, 16 Jan 2024 10:01:47 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCHv5 10/16] x86/tdx: Convert shared memory back to private on
 kexec
Content-Language: en-US
To: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.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
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>
From: Nikolay Borisov <nik.borisov@suse.com>
In-Reply-To: <20240116072822.pvzseyqry56eqa4j@box.shutemov.name>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit


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 ?

>