Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 31 Mar 2022 01:46:10 -0700
From:   Pawan Gupta <pawan.kumar.gupta@linux.intel.com>
To:     Ricardo =?utf-8?Q?Ca=C3=B1uelo?= <ricardo.canuelo@collabora.com>
Cc:     Borislav Petkov <bp@alien8.de>, linux-kernel@vger.kernel.org,
        Thadeu Lima de Souza Cascardo <cascardo@canonical.com>,
        Mark Gross <mgross@linux.intel.com>, x86@kernel.org,
        "H. Peter Anvin" <hpa@zytor.com>, Ingo Molnar <mingo@redhat.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        John Johansen <john.johansen@canonical.com>,
        Steve Beattie <sbeattie@ubuntu.com>, kernel@collabora.com
Subject: Re: [PATCH v2] x86/speculation/srbds: do not try to turn mitigation
 off when not supported
Message-ID: <20220331084536.y4sl7qcfzltsnnew@guptapa-desk>
References: <20220330082026.1549073-1-ricardo.canuelo@collabora.com>
 <YkS3OKLS1Cixs9up@zn.tnic>
 <87o81mzhoh.fsf@rcn-XPS-13-9305.i-did-not-set--mail-host-address--so-tickle-me>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <87o81mzhoh.fsf@rcn-XPS-13-9305.i-did-not-set--mail-host-address--so-tickle-me>
Precedence: bulk

On Thu, Mar 31, 2022 at 09:48:14AM +0200, Ricardo Cañuelo wrote:
>Borislav Petkov <bp@alien8.de> writes:
>> So we could also do the below to denote what the situation is and
>> therefore clear the bug flag for such CPUs.
>>
>> The thing is: I want this to be as clear as possible because bugs.c is
>> already a nightmare and just slapping more logic to it without properly
>> thinking it through is going to be a serious pain to deal with later...
>
>Thanks Boris,
>
>I agree that the more explicit the better, I'll give this a try. I saw
>Pawan's suggestion as well but that one is similar to the originally
>proposed patch in that the logic/checks are split between two functions,
>this solution based on clearing the bug flag seems clearer considering
>the comment just before the code block:
>
>	/*
>	 * Check to see if this is one of the MDS_NO systems supporting
>	 * TSX that are only exposed to SRBDS when TSX is enabled.
>	 */

If we clear the bug flag and not write to the MSR to disable the
microcode mitigation, there will be unnecessary performance loss due to
microcode mitigation. Specifically RDRAND/RDSEED/EGET_KEY will run
slower.

Yes, the logic/checks between two functions is messy.

Does this simplify things a bit?

---
diff --git a/arch/x86/kernel/cpu/bugs.c b/arch/x86/kernel/cpu/bugs.c
index 6296e1ebed1d..0be6c0eabb71 100644
--- a/arch/x86/kernel/cpu/bugs.c
+++ b/arch/x86/kernel/cpu/bugs.c
@@ -431,34 +431,22 @@ static const char * const srbds_strings[] = {
  	[SRBDS_MITIGATION_HYPERVISOR]	= "Unknown: Dependent on hypervisor status",
  };
  
-static bool srbds_off;
+static bool srbds_dis_ucode_mitigation;
  
  void update_srbds_msr(void)
  {
  	u64 mcu_ctrl;
  
-	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
-		return;
-
-	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
-		return;
-
-	if (srbds_mitigation == SRBDS_MITIGATION_UCODE_NEEDED)
+	if (!boot_cpu_has_bug(X86_BUG_SRBDS) ||
+	    !boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
  		return;
  
  	rdmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
  
-	switch (srbds_mitigation) {
-	case SRBDS_MITIGATION_OFF:
-	case SRBDS_MITIGATION_TSX_OFF:
+	if (srbds_dis_ucode_mitigation)
  		mcu_ctrl |= RNGDS_MITG_DIS;
-		break;
-	case SRBDS_MITIGATION_FULL:
+	else
  		mcu_ctrl &= ~RNGDS_MITG_DIS;
-		break;
-	default:
-		break;
-	}
  
  	wrmsrl(MSR_IA32_MCU_OPT_CTRL, mcu_ctrl);
  }
@@ -481,9 +469,13 @@ static void __init srbds_select_mitigation(void)
  		srbds_mitigation = SRBDS_MITIGATION_HYPERVISOR;
  	else if (!boot_cpu_has(X86_FEATURE_SRBDS_CTRL))
  		srbds_mitigation = SRBDS_MITIGATION_UCODE_NEEDED;
-	else if (cpu_mitigations_off() || srbds_off)
+	else if (cpu_mitigations_off())
  		srbds_mitigation = SRBDS_MITIGATION_OFF;
  
+	if (srbds_mitigation == SRBDS_MITIGATION_OFF ||
+	    srbds_mitigation == SRBDS_MITIGATION_TSX_OFF)
+		srbds_dis_ucode_mitigation = true;
+
  	update_srbds_msr();
  	pr_info("%s\n", srbds_strings[srbds_mitigation]);
  }
@@ -496,7 +488,9 @@ static int __init srbds_parse_cmdline(char *str)
  	if (!boot_cpu_has_bug(X86_BUG_SRBDS))
  		return 0;
  
-	srbds_off = !strcmp(str, "off");
+	if (!strcmp(str, "off"))
+		srbds_mitigation = SRBDS_MITIGATION_OFF;
+
  	return 0;
  }
  early_param("srbds", srbds_parse_cmdline);