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:13:30 -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 1iDFXL-0002nO-O0 for speck@linutronix.de; Thu, 26 Sep 2019 00:13:28 +0200 Received: from smtp.corp.redhat.com (int-mx08.intmail.prod.int.phx2.redhat.com [10.5.11.23]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 67B1AA44AC2 for ; Wed, 25 Sep 2019 22:13:21 +0000 (UTC) Received: from treble (ovpn-120-76.rdu2.redhat.com [10.10.120.76]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 16DA219C5B for ; Wed, 25 Sep 2019 22:13:20 +0000 (UTC) Date: Wed, 25 Sep 2019 17:13:19 -0500 From: Josh Poimboeuf Subject: [MODERATED] Re: [PATCH v4 02/10] TAAv4 2 Message-ID: <20190925221319.5h4c3os5ptm6iaft@treble> References: =?utf-8?q?=3C60c982ebc9df0d0642645ff0e555f50b0e4b80a7=2E1567543894=2Egi?= =?utf-8?q?t=2Epawan=2Ekumar=2Egupta=40linux=2Eintel=2Ecom=3E?= <20190904055406.GA7212@kroah.com> <20190904074326.GB1321@guptapadev.amr> MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Fri, Sep 06, 2019 at 02:07:12PM -0700, speck for Dave Hansen wrote: > On 9/4/19 12:43 AM, speck for Pawan Gupta wrote: > > On Wed, Sep 04, 2019 at 07:54:06AM +0200, speck for Greg KH wrote: > >>> +static void tsx_ctrl_check_support(struct cpuinfo_x86 *c) > >>> +{ > >>> + u64 ia32_cap = 0; > >>> + > >>> + if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES)) > >>> + rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap); > >>> + > >>> + if (!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR)) > >>> + tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED; > >> Why isn't this if statement under the if statement above? > > Because we still have to set tsx_ctrl_state when cpu doesn't support > > X86_FEATURE_ARCH_CAPABILITIES. > > FWIW, I struggled to read this the first time I read it, too. It might > make sense to break it up like this: > > u64 read_ia32_arch_cap(void) > { > u64 ia32_cap = 0; > > /* Leave the MSR set to all 0's when not supported */ > if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES)) > rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap); > > return ia32_cap; > } > > You could even replace the other places that we read the MSR. They have > the same pattern. Then do: > > static void tsx_ctrl_check_support(struct cpuinfo_x86 *c) > { > u64 ia32_cap = read_ia32_arch_cap(); > > if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR) > tsx_ctrl_state = TSX_CTRL_ENABLE; > } > > which takes tsx_ctrl_state out of the 'disable' mode. I think it would be even clearer if the compile-time default were changed to TSX_CTRL_NOT_SUPPORTED. Then the logic would be less confusing. For example the reader of the code doesn't have to remember that ia32_cap is initialized to zero. And IMO, "not supported" conceptually makes the most sense as a compile-time default anyway. static void tsx_ctrl_check_support(struct cpuinfo_x86 *c) { u64 ia32_cap; if (cpu_has(c, X86_FEATURE_ARCH_CAPABILITIES)) { rdmsrl(MSR_IA32_ARCH_CAPABILITIES, ia32_cap); if (ia32_cap & ARCH_CAP_TSX_CTRL_MSR) tsx_ctrl_state = TSX_CTRL_DISABLE; } } -- Josh