Messages in this thread | | | Subject | Re: [v3 1/1] x86/cpufeatures: Implement Predictive Store Forwarding control. | From | "Saripalli, RK" <> | Date | Thu, 29 Apr 2021 09:32:35 -0500 |
| |
On 4/29/2021 9:25 AM, Tom Lendacky wrote: > 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.
Yes, I agree with his analysis and fixing it. > > 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 >>>
| |