[MODERATED] Re: [PATCH v6 07/11] [PATCH v6 07/10] Linux Patch #7

[Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>]

On Sat, Apr 28, 2018 at 09:38:29PM +0200, speck for Thomas Gleixner wrote:
> On Thu, 26 Apr 2018, speck for konrad.wilk_at_oracle.com wrote:
> >  void __init check_bugs(void)
> >  {
> > -	identify_boot_cpu();
> > -
> 
> I know that you did this for AMD, but it does not make it less wrong.  The
> right thing to do is to store the MSR_LS_CTR bit somewhere during the
> identification and then use it here. That keeps the fiddling with these
> MSRs in this code and not in some weird latch mode burried in the CPU init
> code.

Please keep in mind that the AP processors still need to engage the latch
mode for the CPU - and they don't don't venture in the 'check_bugs'
code.  Which means "some weird latch mode burried in the CPU init code"
still has to happen.

It could be something as simple as this for BSP:

 43 void __init check_bugs(void)
 44 {
 45         identify_boot_cpu();
 46 
 47         if (!IS_ENABLED(CONFIG_SMP)) {
 48                 pr_info("CPU: ");
 49                 print_cpu_info(&boot_cpu_data);
 50         }
 51 
 52         /*
 53          * Read the SPEC_CTRL MSR to account for reserved bits which may have
 54          * unknown values.
 55          */
 56         if (boot_cpu_has(X86_FEATURE_IBRS))
 57                 rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
 58 
 59         /*
 60          * Select proper mitigation for any exposure to the Speculative Store
 61          * Bypass vulnerability.
 62          */
 63         ssb_select_mitigation();
 64 
 65         if (cpu_has(c, X86_FEATURE_SPEC_STORE_BYPASS_DISABLE)) {
 66                 if (boot_cpu_data.x86_vendor == X86_VENDOR_INTEL) {
 67                         x86_set_spec_ctrl(SPEC_CTRL_RDS);
 68                 } else if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD) {
 69                         u64 val;
 70 
 71                         rdmsrl(MSR_AMD64_LS_CFG, val);
 72                         val |= (1 << x86_msr_ls_ctr_bit);
 73                         wrmsrl(MSR_AMD64_LS_CFG, val);
 74                 }
 75         }

[I actually would likely move this to some function called
 ssb_latch_mitigation_boot_cpu() or ssb_latch_mitigation()]

 76 
 77         /* Select the proper spectre mitigation before patching alternatives */
 78         spectre_v2_select_mitigation();


For the AP processors:

in init_amd:

     if (cpu_has(c, X86_FEATURE_SPEC_STORE_BYPASS_DISABLE))
               msr_set_bit(MSR_AMD64_LS_CFG, x86_msr_ls_ctr_bit);

for the AP CPUs this is needed in init_intel:

 775         if (cpu_has(c, X86_FEATURE_SPEC_STORE_BYPASS_DISABLE))
 776                 x86_set_spec_ctrl(SPEC_CTRL_RDS);


And to figure out which bits to fiddle, for AMD we would do this in
in early_init_amd:

 678         if (c->x86 >= 0x15 && c->x86 <= 0x17) {
 679                 set_cpu_cap(c, X86_FEATURE_RDS);
 680                 switch (c->x86) {
 681                 case 0x15: x86_msr_ls_ctr_bit = 54; break;
 682                 case 0x16: x86_msr_ls_ctr_bit = 33; break;
 683                 case 0x17: x86_msr_ls_ctr_bit = 10; break;
 684                 }
 685         }


for Intel we already identify the X86_FEATURE_RDS thanks to reading
during early init the CPUID.7.edx in get_cpu_cap.

That ties all together..

Taking this one step further - the init_amd or init_intel could
call the ssb_latch_mitigation_boot_cpu() function (which now should be
called ssb_latch_mitigation) - and that would put all the bit/MSR
fiddling code in one location (bugs.c).

I think this is what you had in mind. I will do those changes tomorrow
night and try them out, unless you had in mind a different way?.