Received: by 2002:a25:ab43:0:0:0:0:0 with SMTP id u61csp105599ybi; Thu, 20 Jun 2019 18:51:00 -0700 (PDT) X-Google-Smtp-Source: APXvYqxm8anVjS7TTG+gT0ovfKye0pMXpxQnZxSY96Du0Qb7UnDRbRR5pKouiw4b/6I9wXBXh0Yq X-Received: by 2002:a63:2206:: with SMTP id i6mr16030378pgi.349.1561081860462; Thu, 20 Jun 2019 18:51:00 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1561081860; cv=none; d=google.com; s=arc-20160816; b=SIY0k/v2Cx0rCfQk0RK9JTmwzNgFScMNWDQtaR9qL3+d6Haznr3adPVr9QsBE3JpoI QMF6YA+eI3jyBr9NYRbKEjnVO5GFGjxzawseA0rXnB74UQL/eeTKjcoa9guaJQZ48DxN Uhrmus5hv6ak2rN/aP7e+W6a0EhCHhqTiGO+flS3VLN64JiTwah6a4+tM4HNbRiXGd1V prPEt3iiv3Z7pHlIDYi+D64pu6HMr0Aza8CZo9Ge9yDmBSGm02FXd1qYC2bR/sQJAV6O qcX4ELuzIGknV5G+6Nm+NZiTsv7tyUh41WVhzb+3Cm0xf7gRk3OsUuggxUwCcXNDQpMu cMNQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:mime-version :message-id:to:from:cc:in-reply-to:subject:date; bh=j5EwuhwdKeX+aMQ1HhFBPq9OFCfHqk3KViLFexG5B9E=; b=It5APRe5Oeu0Pjazl2/4vQzMzsuJowhw2j8SVwq63CaBs4+FoAiGMFGIyFeq0v18w9 tgWPFMjLcyFuNM5m+1gnEg5hGjiGr9e/iAZuyQnbT6weEyPkUmI7HK0k17BDyIGZDysv l6EP02cUufWqSXvjhy+kkCTJ+adLlYa+mmTu9g7TYyLzVXmNmLJETcztQhoYpSm2PYhB DypjGrxIDF5O4EaY8Ki2jaDckHPuX9gbjOMTyLCTgh5h9EuN8No7afwKkZIQwx0QlhKB 42E8Me19eYLnp60Tk/YsbOc4a0Yj8VMYoZS8aSwozrgn7NEck/WBXHZul7XgzV/iYxDu Q4lg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id h9si1029907pgq.539.2019.06.20.18.50.44; Thu, 20 Jun 2019 18:51:00 -0700 (PDT) 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; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726017AbfFUBuf (ORCPT + 99 others); Thu, 20 Jun 2019 21:50:35 -0400 Received: from mail-pf1-f193.google.com ([209.85.210.193]:32821 "EHLO mail-pf1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725906AbfFUBue (ORCPT ); Thu, 20 Jun 2019 21:50:34 -0400 Received: by mail-pf1-f193.google.com with SMTP id x15so2713846pfq.0 for ; Thu, 20 Jun 2019 18:50:33 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:subject:in-reply-to:cc:from:to:message-id :mime-version:content-transfer-encoding; bh=j5EwuhwdKeX+aMQ1HhFBPq9OFCfHqk3KViLFexG5B9E=; b=dlFEtWTOXXp+XHAC4reFd5KEDcbxrVaABYW9+9LRPy9RWpq19Tc3Io1A4xpFuMxtkT tK0v/qT+W6S+7L9bQ4humqYxom+6iLdTsAXOMVXGsvH+yKeeR4pFKbqnM/xImvPOpf0c PDdrWsj7xMGQjOKmrA1naJsbU0872KMDwW/WsKqSMyDOGoXZ96m50zdw7Pbht6XJQwWD +HhC2bDbe/sGVrx3x6CfX70SNvCNBr7ebipUmMpwShL+Q6l226P08a3X9pntjCWCCygP aw11wAiBakoJ9kn3PsmixUc9NwQHMlGMI15v0AByJjahO1tL+qEmqdQVuqbrMXgMzKLG PHTw== X-Gm-Message-State: APjAAAWlNd162Lpx1iRAfXH1Xgsn8crHnWe7GCHP5qhFnoPsuiAVaIlS zFbpjViX45q6jvX4Axk643MvcA== X-Received: by 2002:a17:90a:8c0c:: with SMTP id a12mr2983319pjo.67.1561081832818; Thu, 20 Jun 2019 18:50:32 -0700 (PDT) Received: from localhost (amx-tls3.starhub.net.sg. [203.116.164.13]) by smtp.gmail.com with ESMTPSA id h11sm694257pfn.170.2019.06.20.18.50.30 (version=TLS1_3 cipher=AEAD-AES256-GCM-SHA384 bits=256/256); Thu, 20 Jun 2019 18:50:31 -0700 (PDT) Date: Thu, 20 Jun 2019 18:50:31 -0700 (PDT) X-Google-Original-Date: Thu, 20 Jun 2019 18:50:24 PDT (-0700) Subject: Re: [PATCH v2] RISC-V: Break load reservations during switch_to In-Reply-To: <20190619073600.GA29918@lakrids.cambridge.arm.com> CC: linux-riscv@lists.infradead.org, Paul Walmsley , marco@decred.org, me@carlosedp.com, joel@sing.id.au, linux-kernel@vger.kernel.org From: Palmer Dabbelt To: mark.rutland@arm.com Message-ID: 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 19 Jun 2019 00:36:01 PDT (-0700), mark.rutland@arm.com wrote: > On Fri, Jun 07, 2019 at 03:22:22PM -0700, Palmer Dabbelt wrote: >> The comment describes why in detail. This was found because QEMU never >> gives up load reservations, the issue is unlikely to manifest on real >> hardware. >> >> Thanks to Carlos Eduardo for finding the bug! > >> @@ -330,6 +330,17 @@ ENTRY(__switch_to) >> add a3, a0, a4 >> add a4, a1, a4 >> 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. >> + */ > > I suspect that you need to do the same as 32-bit ARM, and clear this in > your exception return path, rather than in __switch_to, since handlers > for interrupts and other exceptions could leave a dangling reservation. > > For ARM, the architecture permits a store-exclusive to succeed even if > the address differed from the load-exclusive. I don't know if the same > applies here, but regardless I believe the case above applies if an IRQ > is taken from kernel context, since the handler can manipulate the same > variable as the interrupted code. RISC-V has the same constraint: an LR can cause the subsequent SC on any address to succeed. When writing the patch I thought they had to have matching addresses, v4 should have a correct comment (assuming I've managed to send it, I'm on my third continent this week so I'm a bit out of it). I'd considered breaking reservations on trap entry, but decided it wasn't necessary. I hadn't considered doing this on trap exit, but I don't see a difference so I might be missing something. The case that I see as an issue is when a trap comes in the middle of an LR/SC sequence, which boils down to three cases: * The trap handler doesn't contend with the LR/SC sequence in any way, in which case it's fine for the sequence to succeed. * The trap handler contends by doing its own LR/SC sequence. Since the trap handler must execute completely before returning control back the parent, we know the SC in the trap handler will execute. Thus there is no way the SC in the parent can pair with the LR in the trap handler. This applies even when traps are nested. * The trap handler contends by performing a regular store to the same address as the LR that was interrupted. In this case the SC must fail, and while I assumed that the store would cause that failure the ISA manual doesn't appear to require that behavior -- it does allow the SC to always fail in that case, but it doesn't mandate it always fails (which is how I got confused). Assuming the ISA manual is correct in not specifying that stores within an LR/SC sequence break the load reservations, then I think we do need to break load reservations on all traps. I'll go check with the ISA people, but now that I've noticed it this does seem somewhat reasonable -- essentially it lets LR just take a line exclusively, SC to succeed only on already exclusively held lines, and doesn't impose any extra constraints on regular memory operations. I don't see any reason that breaking reservations on entry as opposed to exit would be incorrect. I feel like doing this on entry is a better bet, as we're already doing a bunch of stores there so I don't need to bring an additional cache line in for writing. These lines would be on the kernel stack so it's unlikely anyone else has them for writing anyway, so maybe it just doesn't matter. The only issue I can think of here is that there might be something tricky for implementations to handle WRT the regular store -> store conditional forwarding, as that's a pattern that is unlikely to manifest on regular code but is now in a high performance path. The regular load -> store conditional may be easier to handle, and since this is done on the kernel stack it's unlikely that upgrading a line for writing would be an expensive operation anyway. LMK if I'm missing something, otherwise I'll spin another version of the patch to break reservations on trap entry.