From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alex =?utf-8?Q?Benn=C3=A9e?= Subject: Re: [PATCH v10 06/18] arm64: fpsimd: Generalise context saving for non-task contexts Date: Thu, 24 May 2018 10:41:40 +0100 Message-ID: <878t89tmp7.fsf@linaro.org> References: <1527005119-6842-1-git-send-email-Dave.Martin@arm.com> <1527005119-6842-7-git-send-email-Dave.Martin@arm.com> <87in7et9gw.fsf@linaro.org> <20180524090322.GQ13470@e103592.cambridge.arm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-reply-to: <20180524090322.GQ13470@e103592.cambridge.arm.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org To: Dave Martin Cc: Christoffer Dall , Ard Biesheuvel , Marc Zyngier , Catalin Marinas , Will Deacon , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org List-Id: kvmarm@lists.cs.columbia.edu CkRhdmUgTWFydGluIDxEYXZlLk1hcnRpbkBhcm0uY29tPiB3cml0ZXM6Cgo+IE9uIFdlZCwgTWF5 IDIzLCAyMDE4IGF0IDA5OjE1OjExUE0gKzAxMDAsIEFsZXggQmVubsOpZSB3cm90ZToKPj4KPj4g RGF2ZSBNYXJ0aW4gPERhdmUuTWFydGluQGFybS5jb20+IHdyaXRlczoKPj4KPj4gPiBJbiBwcmVw YXJhdGlvbiBmb3IgYWxsb3dpbmcgbm9uLXRhc2sgKGkuZS4sIEtWTSB2Y3B1KSBGUFNJTUQKPj4g PiBjb250ZXh0cyB0byBiZSBoYW5kbGVkIGJ5IHRoZSBmcHNpbWQgY29tbW9uIGNvZGUsIHRoaXMg cGF0Y2ggYWRhcHRzCj4+ID4gdGFza19mcHNpbWRfc2F2ZSgpIHRvIHNhdmUgYmFjayB0aGUgY3Vy cmVudGx5IGxvYWRlZCBjb250ZXh0LAo+PiA+IHJlbW92aW5nIHRoZSBleHBsaWNpdCBkZXBlbmRl bmN5IG9uIGN1cnJlbnQuCj4+ID4KPj4gPiBUaGUgcmVsZXZhbnQgc3RvcmFnZSB0byB3cml0ZSBi YWNrIHRvIGluIG1lbW9yeSBpcyBub3cgZm91bmQgYnkKPj4gPiBleGFtaW5pbmcgdGhlIGZwc2lt ZF9sYXN0X3N0YXRlIHBlcmNwdSBzdHJ1Y3QuCj4+ID4KPj4gPiBmcHNpbWRfc2F2ZSgpIGRvZXMg bm90aGluZyB1bmxlc3MgVElGX0ZPUkVJR05fRlBTVEFURSBpcyBjbGVhciwgYW5kCj4+ID4gZnBz aW1kX2xhc3Rfc3RhdGUgaXMgdXBkYXRlZCB1bmRlciBsb2NhbF9iaF9kaXNhYmxlKCkgb3IKPj4g PiBsb2NhbF9pcnFfZGlzYWJsZSgpIGV2ZXJ5d2hlcmUgdGhhdCBUSUZfRk9SRUlHTl9GUFNUQVRF IGlzIGNsZWFyZWQ6Cj4+ID4gdGh1cywgZnBzaW1kX3NhdmUoKSB3aWxsIHdyaXRlIGJhY2sgdG8g dGhlIGNvcnJlY3Qgc3RvcmFnZSBmb3IgdGhlCj4+ID4gbG9hZGVkIGNvbnRleHQuCj4+ID4KPj4g PiBObyBmdW5jdGlvbmFsIGNoYW5nZS4KPj4gPgo+PiA+IFNpZ25lZC1vZmYtYnk6IERhdmUgTWFy dGluIDxEYXZlLk1hcnRpbkBhcm0uY29tPgo+PiA+IEFja2VkLWJ5OiBNYXJjIFp5bmdpZXIgPG1h cmMuenluZ2llckBhcm0uY29tPgo+PiA+IEFja2VkLWJ5OiBDYXRhbGluIE1hcmluYXMgPGNhdGFs aW4ubWFyaW5hc0Bhcm0uY29tPgo+PiA+IC0tLQo+PiA+ICBhcmNoL2FybTY0L2tlcm5lbC9mcHNp bWQuYyB8IDI1ICsrKysrKysrKysrKystLS0tLS0tLS0tLS0KPj4gPiAgMSBmaWxlIGNoYW5nZWQs IDEzIGluc2VydGlvbnMoKyksIDEyIGRlbGV0aW9ucygtKQo+PiA+Cj4+ID4gZGlmZiAtLWdpdCBh L2FyY2gvYXJtNjQva2VybmVsL2Zwc2ltZC5jIGIvYXJjaC9hcm02NC9rZXJuZWwvZnBzaW1kLmMK Pj4gPiBpbmRleCA5ZDg1MzczLi4zYWExMDBhIDEwMDY0NAo+PiA+IC0tLSBhL2FyY2gvYXJtNjQv a2VybmVsL2Zwc2ltZC5jCj4+ID4gKysrIGIvYXJjaC9hcm02NC9rZXJuZWwvZnBzaW1kLmMKPj4g PiBAQCAtMjcwLDEzICsyNzAsMTUgQEAgc3RhdGljIHZvaWQgdGFza19mcHNpbWRfbG9hZCh2b2lk KQo+PiA+ICB9Cj4+ID4KPj4gPiAgLyoKPj4gPiAtICogRW5zdXJlIGN1cnJlbnQncyBGUFNJTUQv U1ZFIHN0b3JhZ2UgaW4gdGhyZWFkX3N0cnVjdCBpcyB1cCB0byBkYXRlCj4+ID4gLSAqIHdpdGgg cmVzcGVjdCB0byB0aGUgQ1BVIHJlZ2lzdGVycy4KPj4gPiArICogRW5zdXJlIEZQU0lNRC9TVkUg c3RvcmFnZSBpbiBtZW1vcnkgZm9yIHRoZSBsb2FkZWQgY29udGV4dCBpcyB1cCB0bwo+PiA+ICsg KiBkYXRlIHdpdGggcmVzcGVjdCB0byB0aGUgQ1BVIHJlZ2lzdGVycy4KPj4gPiAgICoKPj4gPiAg ICogU29mdGlycXMgKGFuZCBwcmVlbXB0aW9uKSBtdXN0IGJlIGRpc2FibGVkLgo+PiA+ICAgKi8K Pj4gPiAtc3RhdGljIHZvaWQgdGFza19mcHNpbWRfc2F2ZSh2b2lkKQo+PiA+ICtzdGF0aWMgdm9p ZCBmcHNpbWRfc2F2ZSh2b2lkKQo+PiA+ICB7Cj4+ID4gKwlzdHJ1Y3QgdXNlcl9mcHNpbWRfc3Rh dGUgKnN0ID0gX190aGlzX2NwdV9yZWFkKGZwc2ltZF9sYXN0X3N0YXRlLnN0KTsKPj4gPiArCj4+ Cj4+IEkgdGhvdWdodCBJIHdhcyBtaXNzaW5nIHNvbWV0aGluZyBidXQgdGhlIG9ubHkgd3JpdGUg SSBzYXcgb2YgdGhpcyB3YXM6Cj4+Cj4+ICAgX190aGlzX2NwdV93cml0ZShmcHNpbWRfbGFzdF9z dGF0ZS5zdCwgTlVMTCk7Cj4+Cj4+IHdoaWNoIGltcGxpZWQgdG8gbWUgaXQgaXMgcG9zc2libGUg dG8gaGF2ZSBhbiBpbnZhbGlkIGRlLXJlZmVyZW5jZS4gSQo+PiBkaWQgZmlndXJlIGl0IG91dCBl dmVudHVhbGx5IGFzIGZwc2ltZF9iaW5kX3N0YXRlX3RvX2NwdSB1c2VzIGEgbW9yZQo+PiBpbmRp cmVjdCB0aGlzX2NwdV9wdHIgaWRpb20gZm9yIHR3ZWFraW5nIHRoaXMuIEkgZ3Vlc3MgYSByZWZl cmVuY2UgdG8KPj4gZnBzaW1kX2JpbmRfW3Rhc2t8c3RhdGVdX3RvX2NwdSBpbiB0aGUgY29tbWVu dCB3b3VsZCBoYXZlIGhlbHBlZCBteQo+PiBjb25mdXNpb24uCj4KPiBIb3cgYWJvdXQ6Cj4KPiAg c3RhdGljIHZvaWQgZnBzaW1kX3NhdmUodm9pZCkKPiAgewo+ICAJc3RydWN0IHVzZXJfZnBzaW1k X3N0YXRlICpzdCA9IF9fdGhpc19jcHVfcmVhZChmcHNpbWRfbGFzdF9zdGF0ZS5zdCk7Cj4gKwkv KiBzZXQgYnkgZnBzaW1kX2JpbmRfdG9fY3B1KCkgKi8KCkdyZWF0LCB0aGFua3MgOy0pCgotLQpB bGV4IEJlbm7DqWUKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fCmxpbnV4LWFybS1rZXJuZWwgbWFpbGluZyBsaXN0CmxpbnV4LWFybS1rZXJuZWxAbGlzdHMu aW5mcmFkZWFkLm9yZwpodHRwOi8vbGlzdHMuaW5mcmFkZWFkLm9yZy9tYWlsbWFuL2xpc3RpbmZv L2xpbnV4LWFybS1rZXJuZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: alex.bennee@linaro.org (Alex =?utf-8?Q?Benn=C3=A9e?=) Date: Thu, 24 May 2018 10:41:40 +0100 Subject: [PATCH v10 06/18] arm64: fpsimd: Generalise context saving for non-task contexts In-Reply-To: <20180524090322.GQ13470@e103592.cambridge.arm.com> References: <1527005119-6842-1-git-send-email-Dave.Martin@arm.com> <1527005119-6842-7-git-send-email-Dave.Martin@arm.com> <87in7et9gw.fsf@linaro.org> <20180524090322.GQ13470@e103592.cambridge.arm.com> Message-ID: <878t89tmp7.fsf@linaro.org> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Dave Martin writes: > On Wed, May 23, 2018 at 09:15:11PM +0100, Alex Benn?e wrote: >> >> Dave Martin writes: >> >> > In preparation for allowing non-task (i.e., KVM vcpu) FPSIMD >> > contexts to be handled by the fpsimd common code, this patch adapts >> > task_fpsimd_save() to save back the currently loaded context, >> > removing the explicit dependency on current. >> > >> > The relevant storage to write back to in memory is now found by >> > examining the fpsimd_last_state percpu struct. >> > >> > fpsimd_save() does nothing unless TIF_FOREIGN_FPSTATE is clear, and >> > fpsimd_last_state is updated under local_bh_disable() or >> > local_irq_disable() everywhere that TIF_FOREIGN_FPSTATE is cleared: >> > thus, fpsimd_save() will write back to the correct storage for the >> > loaded context. >> > >> > No functional change. >> > >> > Signed-off-by: Dave Martin >> > Acked-by: Marc Zyngier >> > Acked-by: Catalin Marinas >> > --- >> > arch/arm64/kernel/fpsimd.c | 25 +++++++++++++------------ >> > 1 file changed, 13 insertions(+), 12 deletions(-) >> > >> > diff --git a/arch/arm64/kernel/fpsimd.c b/arch/arm64/kernel/fpsimd.c >> > index 9d85373..3aa100a 100644 >> > --- a/arch/arm64/kernel/fpsimd.c >> > +++ b/arch/arm64/kernel/fpsimd.c >> > @@ -270,13 +270,15 @@ static void task_fpsimd_load(void) >> > } >> > >> > /* >> > - * Ensure current's FPSIMD/SVE storage in thread_struct is up to date >> > - * with respect to the CPU registers. >> > + * Ensure FPSIMD/SVE storage in memory for the loaded context is up to >> > + * date with respect to the CPU registers. >> > * >> > * Softirqs (and preemption) must be disabled. >> > */ >> > -static void task_fpsimd_save(void) >> > +static void fpsimd_save(void) >> > { >> > + struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st); >> > + >> >> I thought I was missing something but the only write I saw of this was: >> >> __this_cpu_write(fpsimd_last_state.st, NULL); >> >> which implied to me it is possible to have an invalid de-reference. I >> did figure it out eventually as fpsimd_bind_state_to_cpu uses a more >> indirect this_cpu_ptr idiom for tweaking this. I guess a reference to >> fpsimd_bind_[task|state]_to_cpu in the comment would have helped my >> confusion. > > How about: > > static void fpsimd_save(void) > { > struct user_fpsimd_state *st = __this_cpu_read(fpsimd_last_state.st); > + /* set by fpsimd_bind_to_cpu() */ Great, thanks ;-) -- Alex Benn?e