From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754560AbbCFRds (ORCPT ); Fri, 6 Mar 2015 12:33:48 -0500 Received: from mail-ig0-f182.google.com ([209.85.213.182]:35541 "EHLO mail-ig0-f182.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751099AbbCFRdr (ORCPT ); Fri, 6 Mar 2015 12:33:47 -0500 MIME-Version: 1.0 In-Reply-To: <20150306075833.GA623@gmail.com> References: <54F74F59.5070107@intel.com> <20150305195127.GA12657@redhat.com> <20150305195149.GB12657@redhat.com> <20150305201101.GA21571@gmail.com> <20150305212532.GA16890@redhat.com> <20150306075833.GA623@gmail.com> Date: Fri, 6 Mar 2015 09:33:46 -0800 X-Google-Sender-Auth: dE-a7VzCs3w624jsnz_H8gei9I4 Message-ID: Subject: Re: [PATCH 1/1] x86/fpu: math_state_restore() should not blindly disable irqs From: Linus Torvalds To: Ingo Molnar Cc: Oleg Nesterov , Dave Hansen , Borislav Petkov , Andy Lutomirski , Pekka Riikonen , Rik van Riel , Suresh Siddha , LKML , "Yu, Fenghua" , Quentin Casasnovas Content-Type: multipart/mixed; boundary=047d7b343c9c2cf8580510a216f3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --047d7b343c9c2cf8580510a216f3 Content-Type: text/plain; charset=UTF-8 On Thu, Mar 5, 2015 at 11:58 PM, Ingo Molnar wrote: > > math_state_restore() was historically called with irqs disabled, > because that's how the hardware generates the trap, and also because > back in the days it was possible for it to be an asynchronous > interrupt and interrupt handlers run with irqs off. > > These days it's always an instruction trap, and furthermore it does > inevitably complex things such as memory allocation and signal > processing, which is not done with irqs disabled. > > So keep irqs enabled. I agree with the "keep irqs enabled". However, I do *not* agree with the actual patch, which doesn't do that at all. > @@ -844,8 +844,9 @@ void math_state_restore(void) > { > struct task_struct *tsk = current; > > + local_irq_enable(); > + There's a big difference between "keep interrupts enabled" (ok) and "explicitly enable interrupts in random contexts" (*NOT* ok). So get rid of the "local_irq_enable()" entirely, and replace it with a WARN_ON_ONCE(irqs_disabled()); and let's just fix the cases where this actually gets called with interrupts off. In particular, let's just make the device_not_available thing use a trap gate, not an interrupt gate. And then remove the "conditional_sti()" stuff. IOW, I think the starting point should be something like the attached (which doesn't do the WARN_ON_ONCE() - it should be added for debugging). *NOT* some kind of "let's just enable interrupts blindly" approach. This is completely untested, of course. But I don't see why we should use an interrupt gate for this. Is there anything in "exception_enter()" that requires it? Linus --047d7b343c9c2cf8580510a216f3 Content-Type: text/plain; charset=US-ASCII; name="patch.diff" Content-Disposition: attachment; filename="patch.diff" Content-Transfer-Encoding: base64 X-Attachment-Id: f_i6xv2m300 IGFyY2gveDg2L2tlcm5lbC90cmFwcy5jIHwgMTQgKystLS0tLS0tLS0tLS0KIDEgZmlsZSBjaGFu Z2VkLCAyIGluc2VydGlvbnMoKyksIDEyIGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBhL2FyY2gv eDg2L2tlcm5lbC90cmFwcy5jIGIvYXJjaC94ODYva2VybmVsL3RyYXBzLmMKaW5kZXggOWQyMDcz ZTJlY2M5Li5mMDQ1YWMwMjZmZjEgMTAwNjQ0Ci0tLSBhL2FyY2gveDg2L2tlcm5lbC90cmFwcy5j CisrKyBiL2FyY2gveDg2L2tlcm5lbC90cmFwcy5jCkBAIC04MzYsMTYgKzgzNiwxMiBAQCBhc21s aW5rYWdlIF9fdmlzaWJsZSB2b2lkIF9fYXR0cmlidXRlX18oKHdlYWspKSBzbXBfdGhyZXNob2xk X2ludGVycnVwdCh2b2lkKQogICoKICAqIENhcmVmdWwuLiBUaGVyZSBhcmUgcHJvYmxlbXMgd2l0 aCBJQk0tZGVzaWduZWQgSVJRMTMgYmVoYXZpb3VyLgogICogRG9uJ3QgdG91Y2ggdW5sZXNzIHlv dSAqcmVhbGx5KiBrbm93IGhvdyBpdCB3b3Jrcy4KLSAqCi0gKiBNdXN0IGJlIGNhbGxlZCB3aXRo IGtlcm5lbCBwcmVlbXB0aW9uIGRpc2FibGVkIChlZyB3aXRoIGxvY2FsCi0gKiBsb2NhbCBpbnRl cnJ1cHRzIGFzIGluIHRoZSBjYXNlIG9mIGRvX2RldmljZV9ub3RfYXZhaWxhYmxlKS4KICAqLwog dm9pZCBtYXRoX3N0YXRlX3Jlc3RvcmUodm9pZCkKIHsKIAlzdHJ1Y3QgdGFza19zdHJ1Y3QgKnRz ayA9IGN1cnJlbnQ7CiAKIAlpZiAoIXRza191c2VkX21hdGgodHNrKSkgewotCQlsb2NhbF9pcnFf ZW5hYmxlKCk7CiAJCS8qCiAJCSAqIGRvZXMgYSBzbGFiIGFsbG9jIHdoaWNoIGNhbiBzbGVlcAog CQkgKi8KQEAgLTg1Niw3ICs4NTIsNiBAQCB2b2lkIG1hdGhfc3RhdGVfcmVzdG9yZSh2b2lkKQog CQkJZG9fZ3JvdXBfZXhpdChTSUdLSUxMKTsKIAkJCXJldHVybjsKIAkJfQotCQlsb2NhbF9pcnFf ZGlzYWJsZSgpOwogCX0KIAogCS8qIEF2b2lkIF9fa2VybmVsX2ZwdV9iZWdpbigpIHJpZ2h0IGFm dGVyIF9fdGhyZWFkX2ZwdV9iZWdpbigpICovCkBAIC04ODQsMTggKzg3OSwxMyBAQCBkb19kZXZp Y2Vfbm90X2F2YWlsYWJsZShzdHJ1Y3QgcHRfcmVncyAqcmVncywgbG9uZyBlcnJvcl9jb2RlKQog CWlmIChyZWFkX2NyMCgpICYgWDg2X0NSMF9FTSkgewogCQlzdHJ1Y3QgbWF0aF9lbXVfaW5mbyBp bmZvID0geyB9OwogCi0JCWNvbmRpdGlvbmFsX3N0aShyZWdzKTsKLQogCQlpbmZvLnJlZ3MgPSBy ZWdzOwogCQltYXRoX2VtdWxhdGUoJmluZm8pOwogCQlleGNlcHRpb25fZXhpdChwcmV2X3N0YXRl KTsKIAkJcmV0dXJuOwogCX0KICNlbmRpZgotCW1hdGhfc3RhdGVfcmVzdG9yZSgpOyAvKiBpbnRl cnJ1cHRzIHN0aWxsIG9mZiAqLwotI2lmZGVmIENPTkZJR19YODZfMzIKLQljb25kaXRpb25hbF9z dGkocmVncyk7Ci0jZW5kaWYKKwltYXRoX3N0YXRlX3Jlc3RvcmUoKTsKIAlleGNlcHRpb25fZXhp dChwcmV2X3N0YXRlKTsKIH0KIE5PS1BST0JFX1NZTUJPTChkb19kZXZpY2Vfbm90X2F2YWlsYWJs ZSk7CkBAIC05NTksNyArOTQ5LDcgQEAgdm9pZCBfX2luaXQgdHJhcF9pbml0KHZvaWQpCiAJc2V0 X3N5c3RlbV9pbnRyX2dhdGUoWDg2X1RSQVBfT0YsICZvdmVyZmxvdyk7CiAJc2V0X2ludHJfZ2F0 ZShYODZfVFJBUF9CUiwgYm91bmRzKTsKIAlzZXRfaW50cl9nYXRlKFg4Nl9UUkFQX1VELCBpbnZh bGlkX29wKTsKLQlzZXRfaW50cl9nYXRlKFg4Nl9UUkFQX05NLCBkZXZpY2Vfbm90X2F2YWlsYWJs ZSk7CisJc2V0X3RyYXBfZ2F0ZShYODZfVFJBUF9OTSwgZGV2aWNlX25vdF9hdmFpbGFibGUpOwog I2lmZGVmIENPTkZJR19YODZfMzIKIAlzZXRfdGFza19nYXRlKFg4Nl9UUkFQX0RGLCBHRFRfRU5U UllfRE9VQkxFRkFVTFRfVFNTKTsKICNlbHNlCg== --047d7b343c9c2cf8580510a216f3--