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

[Nikolay Borisov]

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 ?

>