On 17/04/2018 18:26, Christopherson, Sean J wrote:
> On Tue, 2018-04-17, Paolo Bonzini wrote:
>> On 17/04/2018 17:46, Christopherson, Sean J wrote:
>>> On Tue, 2018-04-17, Zdenek Kaspar wrote:
>>>> Hello, I did quick test with latest stable kernel (4.16.2) and got tons
>>>> of vmwrite errors immediately when starting VM:
>>>
>>> Code related to UMIP emulation is effectively doing an unconditional
>>> RMW on SECONDARY_VM_EXEC_CONTROL, which isn't guaranteed to exist on
>>> older processors. KVM already ensures it only advertises UMIP (via
>>> emulation) when SECONDARY_EXEC_DESC can be set, i.e. KVM is already
>>> implicitly checking for SECONDARY_VM_EXEC_CONTROL, so fixing the bug
>>> is just a matter of omitting the unneeded VMREAD/VMWRITE sequence.
>>
>> Thanks for the report!
>>
>> This should be a fix:
>>
>> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
>> index aa66ccd6ed6c..c5dd185825c7 100644
>> --- a/arch/x86/kvm/vmx.c
>> +++ b/arch/x86/kvm/vmx.c
>> @@ -4767,14 +4767,16 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
>> else
>> hw_cr4 |= KVM_PMODE_VM_CR4_ALWAYS_ON;
>>
>> - if ((cr4 & X86_CR4_UMIP) && !boot_cpu_has(X86_FEATURE_UMIP)) {
>> - vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
>> - SECONDARY_EXEC_DESC);
>> - hw_cr4 &= ~X86_CR4_UMIP;
>> - } else if (!is_guest_mode(vcpu) ||
>> - !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC))
>> - vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
>> - SECONDARY_EXEC_DESC);
>> + if (!boot_cpu_has(X86_FEATURE_UMIP) && vmx_umip_emulated()) {
>> + if (cr4 & X86_CR4_UMIP) {
>> + vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
>> + SECONDARY_EXEC_DESC);
>> + hw_cr4 &= ~X86_CR4_UMIP;
>> + } else if (!is_guest_mode(vcpu) ||
>> + !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC))
>> + vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
>> + SECONDARY_EXEC_DESC);
>> + }
>>
>> if (cr4 & X86_CR4_VMXE) {
>> /*
>>
>> I'll test it and send the patch more formally.
>
> Below is what I was thinking for a patch. We should avoid the
> VMREAD/VMWRITE when possible even when we're emulating UMIP.
>
>
> From: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> Subject: [PATCH] KVM: vmx: update SECONDARY_EXEC_DESC only if CR4.UMIP changes
>
> Update SECONDARY_EXEC_DESC in SECONDARY_VM_EXEC_CONTROL for UMIP
> emulation if and only if CR4.UMIP is being modified and UMIP is
> not supported by hardware, i.e. we're emulating UMIP. If CR4.UMIP
> is not being changed then it's safe to assume that the previous
> invocation of vmx_set_cr4() correctly set SECONDARY_EXEC_DESC,
> i.e. the desired value is already the current value. This avoids
> unnecessary VMREAD/VMWRITE to SECONDARY_VM_EXEC_CONTROL, which
> is critical as not all processors support SECONDARY_VM_EXEC_CONTROL.
>
> WARN once and signal a fault if CR4.UMIP is changing and UMIP can't
> be emulated, i.e. SECONDARY_EXEC_DESC can't be set. Prior checks
> should prevent setting UMIP if it can't be emulated, i.e. UMIP
> shouldn't have been advertised to the guest if it can't be emulated,
> regardless of whether or not UMIP is supported in bare metal.
>
> Fixes: 0367f205a3b7 ("KVM: vmx: add support for emulating UMIP")
> Signed-off-by: Sean Christopherson <sean.j.christopherson@xxxxxxxxx>
> ---
> arch/x86/kvm/vmx.c | 34 ++++++++++++++++++++--------------
> 1 file changed, 20 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index aafcc9881e88..31b36b9801bb 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -1494,6 +1494,12 @@ static inline bool cpu_has_vmx_vmfunc(void)
> SECONDARY_EXEC_ENABLE_VMFUNC;
> }
>
> +static bool vmx_umip_emulated(void)
> +{
> + return vmcs_config.cpu_based_2nd_exec_ctrl &
> + SECONDARY_EXEC_DESC;
> +}
> +
> static inline bool report_flexpriority(void)
> {
> return flexpriority_enabled;
> @@ -4776,14 +4782,20 @@ static int vmx_set_cr4(struct kvm_vcpu *vcpu, unsigned long cr4)
> else
> hw_cr4 |= KVM_PMODE_VM_CR4_ALWAYS_ON;
>
> - if ((cr4 & X86_CR4_UMIP) && !boot_cpu_has(X86_FEATURE_UMIP)) {
> - vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
> - SECONDARY_EXEC_DESC);
> - hw_cr4 &= ~X86_CR4_UMIP;
> - } else if (!is_guest_mode(vcpu) ||
> - !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC))
> - vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> - SECONDARY_EXEC_DESC);
> + if (((cr4 ^ kvm_read_cr4(vcpu)) & X86_CR4_UMIP) &&
> + !boot_cpu_has(X86_FEATURE_UMIP)) {
> + if (WARN_ON_ONCE(!vmx_umip_emulated()))
> + return 1;
> +
> + if (cr4 & X86_CR4_UMIP) {
> + vmcs_set_bits(SECONDARY_VM_EXEC_CONTROL,
> + SECONDARY_EXEC_DESC);
> + hw_cr4 &= ~X86_CR4_UMIP;
> + } else if (!is_guest_mode(vcpu) ||
> + !nested_cpu_has2(get_vmcs12(vcpu), SECONDARY_EXEC_DESC))
> + vmcs_clear_bits(SECONDARY_VM_EXEC_CONTROL,
> + SECONDARY_EXEC_DESC);
> + }
Yes, that's nice too!
Paolo
