Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 06 Jun 2019 12:10:31 -0700 (PDT)
Subject:     Re: [PATCH] RISC-V: Break load reservations during switch_to
In-Reply-To: <20190606090518.GB1369@infradead.org>
CC:     linux-riscv@lists.infradead.org,
        Paul Walmsley <paul.walmsley@sifive.com>, marco@decred.org,
        me@carlosedp.com, joel@sing.id.au, linux-kernel@vger.kernel.org
From:   Palmer Dabbelt <palmer@sifive.com>
To:     Christoph Hellwig <hch@infradead.org>
Message-ID: <mhng-ea7cfd57-8e3a-49e6-8f95-74c009001774@palmer-si-x1c4>
Mime-Version: 1.0 (MHng)
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, 06 Jun 2019 02:05:18 PDT (-0700), Christoph Hellwig wrote:
> On Wed, Jun 05, 2019 at 04:17:35PM -0700, Palmer Dabbelt wrote:
>>  	REG_S ra,  TASK_THREAD_RA_RA(a3)
>> +	/*
>> +	 * The Linux ABI allows programs to depend on load reservations being
>> +	 * broken on context switches, but the ISA doesn't require that the
>> +	 * hardware ever breaks a load reservation.  The only way to break a
>> +	 * load reservation is with a store conditional, so we emit one here.
>> +	 * Since nothing ever takes a load reservation on TASK_THREAD_RA_RA we
>> +	 * know this will always fail, but just to be on the safe side this
>> +	 * writes the same value that was unconditionally written by the
>> +	 * previous instruction.
>> +	 */
>> +#if (TASK_THREAD_RA_RA != 0)
>
> I don't think this check works as intended.  TASK_THREAD_RA_RA is a
> parameterized macro, thus the above would never evaluate to 0. The
> error message also is rather odd while we're at it.

OK, I didn't try it.  The resulting number can never be non-zero, which is why
I couldn't come up with an error message that made sense.  I'm not opposed to
dropping the check.

>> +#if (__riscv_xlen == 64)
>> +	sc.d  x0, ra, 0(a3)
>> +#else
>> +	sc.w  x0, ra, 0(a3)
>> +#endif
>
> I'd rather add an macro ala REG_S to asm.h and distinguish between the
> xlen variants there:
>
> #define REG_SC		__REG_SEL(sc.d, sc.w)

Ya, I guess I was just being lazy.  I'll put that in a v2.