Jim Mattson <jmattson@xxxxxxxxxx> writes:
> On Wed, Jun 29, 2022 at 8:07 AM Vitaly Kuznetsov <vkuznets@xxxxxxxxxx> wrote:
>>
>> As a preparation to reusing the result of setup_vmcs_config() for setting
>> up nested VMX control MSRs, move LOAD_IA32_PERF_GLOBAL_CTRL errata handling
>> to vmx_vmexit_ctrl()/vmx_vmentry_ctrl() and print the warning from
>> hardware_setup(). While it seems reasonable to not expose
>> LOAD_IA32_PERF_GLOBAL_CTRL controls to L1 hypervisor on buggy CPUs,
>> such change would inevitably break live migration from older KVMs
>> where the controls are exposed. Keep the status quo for know, L1 hypervisor
>> itself is supposed to take care of the errata.
>
> It can only do that if L1 doesn't lie about the model. This is why
> F/M/S checks are, in general, evil.
>
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@xxxxxxxxxx>
>> ---
>> arch/x86/kvm/vmx/vmx.c | 62 ++++++++++++++++++++++++++----------------
>> 1 file changed, 38 insertions(+), 24 deletions(-)
>>
>> diff --git a/arch/x86/kvm/vmx/vmx.c b/arch/x86/kvm/vmx/vmx.c
>> index fb58b0be953d..5f7ef1f8d2c6 100644
>> --- a/arch/x86/kvm/vmx/vmx.c
>> +++ b/arch/x86/kvm/vmx/vmx.c
>> @@ -2416,6 +2416,31 @@ static bool cpu_has_sgx(void)
>> return cpuid_eax(0) >= 0x12 && (cpuid_eax(0x12) & BIT(0));
>> }
>>
>> +/*
>> + * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they
>> + * can't be used due to an errata where VM Exit may incorrectly clear
>
> Nit: erratum (singular), or drop the 'an' to refer to errata (plural).
>
>> + * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the
>
> Nit: workaround (one word) is a noun. The verb form is "work around."
>
Sure, but I'm not exactly certain which commit to blame here as I have
options:
Fixes: 8bf00a529967 ("KVM: VMX: add support for switching of PERF_GLOBAL_CTRL")
where it was introduced or
Fixes: bb3541f175a9 ("KVM: x86: Fix typos")
Fixes: c73da3fcab43 ("KVM: VMX: Properly handle dynamic VM Entry/Exit controls")
where it was preserved :-)
>> + * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL.
>> + */
>> +static bool cpu_has_perf_global_ctrl_bug(void)
>> +{
>> + if (boot_cpu_data.x86 == 0x6) {
>> + switch (boot_cpu_data.x86_model) {
>> + case 26: /* AAK155 */
>> + case 30: /* AAP115 */
>> + case 37: /* AAT100 */
>> + case 44: /* BC86,AAY89,BD102 */
>> + case 46: /* BA97 */
>
> Nit: Replace decimal model numbers with mnemonics. See
> https://lore.kernel.org/kvm/20220629222221.986645-1-jmattson@xxxxxxxxxx/.
>
I'm going to steal your patch and put it to my series (as I don't see it
in kvm/queue yet).
>> + return true;
>> + default:
>> + break;
>> + }
>> + }
>> +
>> + return false;
>> +}
>
> Is it worth either (a) memoizing the result, or (b) toggling a static
> branch? Or am I prematurely optimizing?
>
(Unless I missed something) besides hardware_setup(),
cpu_has_perf_global_ctrl_bug() is only called (twice) from:
vmx_vcpu_reset()
__vmx_vcpu_reset()
init_vmcs()
vmx_vmentry_ctrl()
vmx_vmexit_ctrl()
this shouldn't happen very often so I guess we can leave it
un-optimized. Also, we're only reading boot_cpu_data.x86/
boot_cpu_data.x86_model here, this should be cheap).
>> +
>> +
>> static __init int adjust_vmx_controls(u32 ctl_min, u32 ctl_opt,
>> u32 msr, u32 *result)
>> {
>> @@ -2572,30 +2597,6 @@ static __init int setup_vmcs_config(struct vmcs_config *vmcs_conf,
>> _vmexit_control &= ~x_ctrl;
>> }
>>
>> - /*
>> - * Some cpus support VM_{ENTRY,EXIT}_IA32_PERF_GLOBAL_CTRL but they
>> - * can't be used due to an errata where VM Exit may incorrectly clear
>> - * IA32_PERF_GLOBAL_CTRL[34:32]. Workaround the errata by using the
>> - * MSR load mechanism to switch IA32_PERF_GLOBAL_CTRL.
>> - */
>> - if (boot_cpu_data.x86 == 0x6) {
>> - switch (boot_cpu_data.x86_model) {
>> - case 26: /* AAK155 */
>> - case 30: /* AAP115 */
>> - case 37: /* AAT100 */
>> - case 44: /* BC86,AAY89,BD102 */
>> - case 46: /* BA97 */
>> - _vmentry_control &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
>> - _vmexit_control &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
>> - pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL "
>> - "does not work properly. Using workaround\n");
>> - break;
>> - default:
>> - break;
>> - }
>> - }
>> -
>> -
>> rdmsr(MSR_IA32_VMX_BASIC, vmx_msr_low, vmx_msr_high);
>>
>> /* IA-32 SDM Vol 3B: VMCS size is never greater than 4kB. */
>> @@ -4188,6 +4189,10 @@ static u32 vmx_vmentry_ctrl(void)
>> VM_ENTRY_LOAD_IA32_EFER |
>> VM_ENTRY_IA32E_MODE);
>>
>> +
>> + if (cpu_has_perf_global_ctrl_bug())
>> + vmentry_ctrl &= ~VM_ENTRY_LOAD_IA32_PERF_GLOBAL_CTRL;
>> +
>> return vmentry_ctrl;
>> }
>>
>> @@ -4202,6 +4207,10 @@ static u32 vmx_vmexit_ctrl(void)
>> if (vmx_pt_mode_is_system())
>> vmexit_ctrl &= ~(VM_EXIT_PT_CONCEAL_PIP |
>> VM_EXIT_CLEAR_IA32_RTIT_CTL);
>> +
>> + if (cpu_has_perf_global_ctrl_bug())
>> + vmexit_ctrl &= ~VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL;
>> +
>> /* Loading of EFER and PERF_GLOBAL_CTRL are toggled dynamically */
>> return vmexit_ctrl &
>> ~(VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL | VM_EXIT_LOAD_IA32_EFER);
>> @@ -8117,6 +8126,11 @@ static __init int hardware_setup(void)
>> if (setup_vmcs_config(&vmcs_config, &vmx_capability) < 0)
>> return -EIO;
>>
>> + if (cpu_has_perf_global_ctrl_bug()) {
>> + pr_warn_once("kvm: VM_EXIT_LOAD_IA32_PERF_GLOBAL_CTRL "
>> + "does not work properly. Using workaround\n");
>> + }
>> +
>> if (boot_cpu_has(X86_FEATURE_NX))
>> kvm_enable_efer_bits(EFER_NX);
>>
>> --
>> 2.35.3
>>
>
--
Vitaly
