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 ; 26 Sep 2019 07:21:25 -0000 Received: from mga12.intel.com ([192.55.52.136]) by Galois.linutronix.de with esmtps (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1iDO5c-0001JO-7E for speck@linutronix.de; Thu, 26 Sep 2019 09:21:24 +0200 Date: Thu, 26 Sep 2019 00:15:55 -0700 From: Pawan Gupta Subject: [MODERATED] Re: [PATCH v4 03/10] TAAv4 3 Message-ID: <20190926071554.GA12539@guptapadev.amr> References: <5b6df5ee-a5b7-c281-de29-af6544b8abb6@intel.com> <20190906074645.GE13480@guptapadev.amr> <20190925224817.zcmzswojw5tmymul@treble> <20190926011305.GD16811@guptapadev.amr> <20190926023445.afjeaizeloehi6bn@treble> MIME-Version: 1.0 In-Reply-To: <20190926023445.afjeaizeloehi6bn@treble> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit To: speck@linutronix.de List-ID: On Wed, Sep 25, 2019 at 09:34:45PM -0500, speck for Josh Poimboeuf wrote: > On Wed, Sep 25, 2019 at 06:13:05PM -0700, speck for Pawan Gupta wrote: > > > > > > +static int __init tsx_cmdline(char *str) > > > > > > +{ > > > > > > + if (!str) > > > > > > + return -EINVAL; > > > > > > + > > > > > > + if (!strcmp(str, "on")) > > > > > > + tsx_ctrl_state = TSX_CTRL_ENABLE; > > > > > > + else if (!strcmp(str, "off")) > > > > > > + tsx_ctrl_state = TSX_CTRL_DISABLE; > > > > > > + > > > > > > + return 0; > > > > > > +} > > > > > > +early_param("tsx", tsx_cmdline); > > > > > > > > > > Hmm... Let's say I have a non-TSX system. I specify tsx=on. This will > > > > > set tsx_ctrl_state=TSX_CTRL_ENABLE, then tsx_ctrl_check_support() will > > > > > set it to tsx_ctrl_state=TSX_CTRL_NOT_SUPPORTED. > > > > > > > > > > I *think* this all works, but I'd love a comment or two about it. Maybe > > > > > even something that makes it clear that tsx_en/disable() are both only > > > > > called when TSX *AND* TSX_CTRL_MSR are supported. > > > > > > > > > > This is all kinda weird and confusing because we're doing this all > > > > > without the X86_FEATURE bits for TSX. > > > > > > > > If we add another X86_FEATURE bit it will have exact same state as > > > > X86_FEATURE_RTM. Do we really need this? > > > > > > How about moving the tsx_init() call to early_init_intel()? Then you'd > > > be able to check the feature bit above. And that might make the > > > ordering a little easier to follow. > > > > Sorry, I am failing to understand how it would make a difference. > > early_init_intel() is also called from init_init(). > > Regardless, shouldn't tsx_init() at least be called before > cpu_set_bug_bits(), so that X86_BUG_TAA can get set appropriately? X86_BUG_TAA should be set even when tsx_init() disables TSX. This is to indicate that the CPU has the bug but was mitigated by disabling TSX. That is why we are calling cpu_set_bug_bits() first to set X86_BUG_TAA and then disabling TSX in tsx_init(). Moreover cpu_set_bug_bits() is called only by the boot-cpu, and tsx_init() is called per-cpu to disable TSX on each CPU. > In that case, instead of early_param(), tsx_init() could just use > cmdline_find_option() to find the 'tsx=' option. That would also have > the advantage of making it much easier to untangle the initialization > order. Thanks for the suggestion. Does this look okay? static enum tsx_ctrl_states { TSX_CTRL_ENABLE, TSX_CTRL_DISABLE, TSX_CTRL_NOT_SUPPORTED, } tsx_ctrl_state = TSX_CTRL_NOT_SUPPORTED; static bool tsx_ctrl_is_supported(void) { u64 ia32_cap = read_ia32_arch_cap(); /* * TSX is controlled via MSR_IA32_TSX_CTRL. However, * support for this MSR is enumerated by ARCH_CAP_TSX_MSR bit * in MSR_IA32_ARCH_CAPABILITIES. */ return !!(ia32_cap & ARCH_CAP_TSX_CTRL_MSR); } void tsx_init(struct cpuinfo_x86 *c) { char arg[20]; int ret; /* return if TSX_CTRL is not supported */ if (!tsx_ctrl_is_supported()) return; ret = cmdline_find_option(boot_command_line, "tsx", arg, sizeof(arg)); if (ret >= 0) { if (!strcmp(arg, "on")) tsx_ctrl_state = TSX_CTRL_ENABLE; else if (!strcmp(arg, "off")) tsx_ctrl_state = TSX_CTRL_DISABLE; else pr_info("tsx: invalid option, defaulting to off\n"); } /* * If user provided an invalid option or tsx= is not provided on cmdline, * default to TSX_CTRL_DISABLE. This is because on certain processors * TSX may be used as part of a speculative side channel attack. */ if (tsx_ctrl_state == TSX_CTRL_NOT_SUPPORTED) tsx_ctrl_state = TSX_CTRL_DISABLE; switch (tsx_ctrl_state) { case TSX_CTRL_DISABLE: tsx_disable(); [...] > If early_init_intel() isn't the right spot, then maybe > early_identify_cpu(). early_identify_cpu() is called for other vendors as well, I think init_intel() is the ideal place for calling an Intel specific function. Why do we want to move tsx_init() from init_intel()? Thanks, Pawan