From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linutronix.de (193.142.43.55:993) by crypto-ml.lab.linutronix.de with IMAP4-SSL for ; 25 Sep 2019 22:30:28 -0000 Received: from mx1.redhat.com ([209.132.183.28]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iDFnj-00032j-NA for speck@linutronix.de; Thu, 26 Sep 2019 00:30:25 +0200 Received: from smtp.corp.redhat.com (int-mx01.intmail.prod.int.phx2.redhat.com [10.5.11.11]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id E449B3058E0A for ; Wed, 25 Sep 2019 22:30:16 +0000 (UTC) Received: from treble (ovpn-120-76.rdu2.redhat.com [10.10.120.76]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 8BF68600C8 for ; Wed, 25 Sep 2019 22:30:16 +0000 (UTC) Date: Wed, 25 Sep 2019 17:30:14 -0500 From: Josh Poimboeuf Subject: [MODERATED] Re: [PATCH v4 02/10] TAAv4 2 Message-ID: <20190925223014.3diomop66p4efm36@treble> References: =?utf-8?q?=3C60c982ebc9df0d0642645ff0e555f50b0e4b80a7=2E1567543894=2Egi?= =?utf-8?q?t=2Epawan=2Ekumar=2Egupta=40linux=2Eintel=2Ecom=3E?= MIME-Version: 1.0 In-Reply-To: =?utf-8?q?=3C60c982ebc9df0d0642645ff0e555f50b0e4b80a7=2E15675?= =?utf-8?q?43894=2Egit=2Epawan=2Ekumar=2Egupta=40linux=2Eintel=2Ecom=3E?= Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Tue, Sep 03, 2019 at 02:12:32PM -0700, speck for Pawan Gupta wrote: > From: Pawan Gupta > Subject: [PATCH v4 02/10] x86/tsx: Add TSX control initialization > > Add a new file to host TSX feature controls. TSX control has three "Add a new file" makes it sound like a sysfs file rather than a .c file. And it would probably be better to lead off with a description of the *functional* change of the patch, rather than the not-so-interesting aspect of adding a .c file. For example, this patch disables TSX by default. That should also be reflected in the patch subject. (This is of course pending the other discussion about what defaults make sense.) > valid states ENABLE, DISABLE and NOT_SUPPORTED. TSX may be used on > certain processors as part of a speculative side channel attack. How is that relevant to this patch? > Set the boot time default to DISABLE or NOT_SUPPORTED based on the > presence of IA32_TSX_CTRL MSR. This should say *why* it's changing the default to disabled, assuming that's what we decide to do. > +void tsx_init(struct cpuinfo_x86 *c) > +{ > + tsx_ctrl_check_support(c); > + > + switch (tsx_ctrl_state) { > + case TSX_CTRL_DISABLE: > + tsx_disable(); > + clear_cpu_cap(c, X86_FEATURE_RTM); > + setup_clear_cpu_cap(X86_FEATURE_RTM); Can you clarify why setup_clear_cpu_cap() is needed? AFAICT, setup_clear_cpu_cap() would be used for forcing a feature to be cleared on all CPUs, but this function already calls clear_cpu_cap() for every CPU anyway. -- Josh