Re: [v3 1/1] x86/cpufeatures: Implement Predictive Store Forwarding control.

[Tom Lendacky <thomas.lendacky@amd.com>]

On 4/29/21 9:01 AM, Saripalli, RK wrote:
> 
> 
> On 4/29/2021 12:38 AM, Reiji Watanabe wrote:
>>> +       if (!strcmp(str, "off")) {
>>> +               set_cpu_cap(&boot_cpu_data, X86_FEATURE_MSR_SPEC_CTRL);
>>> +               x86_spec_ctrl_base |= SPEC_CTRL_PSFD;
>>> +               wrmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>>
>> My previous suggestion about updating MSR_IA32_SPEC_CTRL meant
>> something like:
>>
>>     rdmsrl(MSR_IA32_SPEC_CTRL, current);
>>     wrmsrl(MSR_IA32_SPEC_CTRL, current | SPEC_CTRL_PSFD);
>>
>> And this is to keep the behavior of code below check_bugs().
>> (Or do you intentionally change it due to some reason ?)
>> BTW, x86_spec_ctrl_base, which is updated in psf_cmdline(),
>> would be overwritten by check_bugs() anyway as follows.
>> ---
> 
> Since psf_cmdline() directly writes to the MSR itself (and it only does this)
> if the feature is available (per CPUID), check_bugs() should be ok.
> 
> My patch for now does not depend on the value of x86_spec_ctrl_base after psf_cmdline()
> finishes execution.

Reiji is correct. What if BIOS has set some other bits in SPEC_CTRL (now
or in the future) as part of setup. You will effectively be zeroing them
out. The correct method is as he has documented, by reading the MSR,
or'ing in the PSFD bit and writing the MSR.

Thanks,
Tom

> 
>> void __init check_bugs(void)
>> {
>>         <...>
>>         /*
>>          * Read the SPEC_CTRL MSR to account for reserved bits which may
>>          * have unknown values. AMD64_LS_CFG MSR is cached in the early AMD
>>          * init code as it is not enumerated and depends on the family.
>>          */
>>         if (boot_cpu_has(X86_FEATURE_MSR_SPEC_CTRL))
>>                 rdmsrl(MSR_IA32_SPEC_CTRL, x86_spec_ctrl_base);
>>         <...>
>> ---
>>
>>> +               setup_clear_cpu_cap(X86_FEATURE_PSFD);
>>
>> Does X86_FEATURE_PSFD need to be cleared for the 'off' case ?
>> Do you want to remove "psfd" from /proc/cpuinfo
>> when PSFD is enabled ? (not when PSFD is disabled ?)
>>
>>
> No, it should not be cleared, I agree.
> But I did test with KVM (with my patch that is not here) and I do not see
> issues (meaning user space guest in QEMU is seeing PSF CPUID guest capability)
> 
> I see no reason to clear this feature and I will submit a new patch with this and other changes.
> 
>> Thanks,
>> Reiji
>>