Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Tom Lendacky <thomas.lendacky@amd.com>,
        Tony Battersby <tonyb@cybernetics.com>,
        Dave Hansen <dave.hansen@intel.com>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>, x86@kernel.org
Cc:     "H. Peter Anvin" <hpa@zytor.com>,
        Mario Limonciello <mario.limonciello@amd.com>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH RFC] x86/cpu: fix intermittent lockup on poweroff
In-Reply-To: <01a44722-931a-7aff-4f4b-75e78855beb1@amd.com>
References: <3817d810-e0f1-8ef8-0bbd-663b919ca49b@cybernetics.com>
 <f5c7a104-d422-bd02-d361-e9e9f433d41d@intel.com> <87o7nbzn8w.ffs@tglx>
 <5f8a9cb8-70cf-2a17-cfc4-cb31cb658de4@cybernetics.com>
 <87y1mey503.ffs@tglx>
 <ccf57fd2-45b8-1f1f-f46a-55d7f4c56161@cybernetics.com>
 <01a44722-931a-7aff-4f4b-75e78855beb1@amd.com>
Date:   Wed, 26 Apr 2023 22:00:33 +0200
Message-ID: <87sfcmxvku.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Wed, Apr 26 2023 at 12:51, Tom Lendacky wrote:
> On 4/26/23 12:37, Tony Battersby wrote:
>>> +	/*
>>> +	 * native_stop_other_cpus() will write to @stop_cpus_count after
>>> +	 * observing that it went down to zero, which will invalidate the
>>> +	 * cacheline on this CPU.
>>> +	 */
>>> +	atomic_dec(&stop_cpus_count);
>
> This is probably going to pull in a cache line and cause the problem the 
> native_wbinvd() is trying to avoid.

The comment above this atomic_dec() explains why this is _not_ a
problem. Here is the counterpart in native_stop_other_cpus():

>>> @@ -216,6 +219,12 @@ static void native_stop_other_cpus(int w
>>>   	disable_local_APIC();
>>>   	mcheck_cpu_clear(this_cpu_ptr(&cpu_info));
>>>   	local_irq_restore(flags);
>>> +
>>> +	/*
>>> +	 * Ensure that the cache line is invalidated on the other CPUs. See
>>> +	 * comment vs. SME in stop_this_cpu().
>>> +	 */
>>> +	atomic_set(&stop_cpus_count, INT_MAX);

That happens _after_ all the other CPUs did the atomic_dec() as the
control CPU waits for it to become 0.

As this makes the cacheline exclusive on the control CPU the dirty
cacheline on the CPU which did the last atomic_dec() is invalidated.

As the atomic_dec() is obviously serialized via the lock prefix there
can be only one dirty copy on some other CPU at the time when the
control CPU writes to it.

After that the only dirty copy is on the control CPU, no?

Thanks,

        tglx