linux-kernel - Re: [PATCH] KVM: nSVM: Optimize L12 to L2 vmcb.save copies

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <2af5432a-6385-b32e-a69d-edfdccd7273b@redhat.com>
Date:   Tue, 2 Mar 2021 15:18:08 +0100
From:   Paolo Bonzini <pbonzini@...hat.com>
To:     Cathy Avery <cavery@...hat.com>,
        Sean Christopherson <seanjc@...gle.com>
Cc:     linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        vkuznets@...hat.com, wei.huang2@....com
Subject: Re: [PATCH] KVM: nSVM: Optimize L12 to L2 vmcb.save copies

On 02/03/21 13:56, Cathy Avery wrote:
> On 3/1/21 7:59 PM, Sean Christopherson wrote:
>> On Mon, Mar 01, 2021, Cathy Avery wrote:
>  	svm->nested.vmcb12_gpa = 0;
> +	svm->nested.last_vmcb12_gpa = 0;


This should not be 0 to avoid a false match.  "-1" should be okay.

>>>       kvm_set_rflags(&svm->vcpu, vmcb12->save.rflags | 
>>> X86_EFLAGS_FIXED);
>>>       svm_set_efer(&svm->vcpu, vmcb12->save.efer);
>>>       svm_set_cr0(&svm->vcpu, vmcb12->save.cr0);
>>>       svm_set_cr4(&svm->vcpu, vmcb12->save.cr4);
>> Why not utilize VMCB_CR?
> I was going to tackle CR in a follow up patch. I should have mentioned 
> that but it makes sense to go ahead and do it now.

There is some trickiness.  For example, I would first prefer to move the 
checks on svm->vmcb->save.cr0 == vcpu->arch.cr0 ("hcr0 == cr0" in 
svm_set_cr0) to recalc_intercepts.

For cr4, instead, we need to go through kvm_update_cpuid_runtime in case 
host CR4 is not equal to CR4 (for which we have a testcase in svm.flat 
already, I think).

>>> -    svm->vcpu.arch.cr2 = vmcb12->save.cr2;
>>> +    svm->vmcb->save.cr2 = svm->vcpu.arch.cr2 = vmcb12->save.cr2;
>> Same question for VMCB_CR2.
>>
>> Also, isn't writing svm->vmcb->save.cr2 unnecessary since svm_vcpu_run()
>> unconditionally writes it?
>>
>> Alternatively, it shouldn't be too much work to add proper dirty 
>> tracking for CR2.  VMX has to write the real CR2 every time because there's no VMCS 
>> field, but I assume can avoid the write and dirty update on the majority of 
>> VMRUNs.
> 
> I'll take a look at CR2 as well.

That's a separate patch, to some extent unrelated to nesting.  Feel free 
to look at it, but for now we should apply this part with only the 
svm->vmcb->save.cr2 assignment removed.  Please send a v2, thanks!

Paolo

> Thanks for the feedback,
> 
> Cathy
> 
>>
>>> +
>>>       kvm_rax_write(&svm->vcpu, vmcb12->save.rax);
>>>       kvm_rsp_write(&svm->vcpu, vmcb12->save.rsp);
>>>       kvm_rip_write(&svm->vcpu, vmcb12->save.rip);
>>>       /* In case we don't even reach vcpu_run, the fields are not 
>>> updated */
>>> -    svm->vmcb->save.cr2 = svm->vcpu.arch.cr2;
>>>       svm->vmcb->save.rax = vmcb12->save.rax;
>>>       svm->vmcb->save.rsp = vmcb12->save.rsp;
>>>       svm->vmcb->save.rip = vmcb12->save.rip;
>>> -    svm->vmcb->save.dr7 = vmcb12->save.dr7 | DR7_FIXED_1;
>>> -    svm->vcpu.arch.dr6  = vmcb12->save.dr6 | DR6_ACTIVE_LOW;
>>> -    vmcb_mark_dirty(svm->vmcb, VMCB_DR);
>>> +    /* These bits will be set properly on the first execution when 
>>> new_vmc12 is true */
>>> +    if (unlikely(new_vmcb12 || vmcb_is_dirty(vmcb12, VMCB_DR))) {
>>> +        svm->vmcb->save.dr7 = vmcb12->save.dr7 | DR7_FIXED_1;
>>> +        svm->vcpu.arch.dr6  = vmcb12->save.dr6 | DR6_ACTIVE_LOW;
>>> +        vmcb_mark_dirty(svm->vmcb, VMCB_DR);
>>> +    }
>>>   }
>>>   static void nested_vmcb02_prepare_control(struct vcpu_svm *svm)
>>> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
>>> index 54610270f66a..9761a7ca8100 100644
>>> --- a/arch/x86/kvm/svm/svm.c
>>> +++ b/arch/x86/kvm/svm/svm.c
>>> @@ -1232,6 +1232,7 @@ static void init_vmcb(struct kvm_vcpu *vcpu)
>>>       svm->asid = 0;
>>>       svm->nested.vmcb12_gpa = 0;
>>> +    svm->nested.last_vmcb12_gpa = 0;
>> We should use INVALID_PAGE, '0' is a legal physical address and could
>> theoretically get a false negative on the "new_vmcb12" check.
>>
>>>       vcpu->arch.hflags = 0;
>>>       if (!kvm_pause_in_guest(vcpu->kvm)) {
>>> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
>>> index fbbb26dd0f73..911868d4584c 100644
>>> --- a/arch/x86/kvm/svm/svm.h
>>> +++ b/arch/x86/kvm/svm/svm.h
>>> @@ -93,6 +93,7 @@ struct svm_nested_state {
>>>       u64 hsave_msr;
>>>       u64 vm_cr_msr;
>>>       u64 vmcb12_gpa;
>>> +    u64 last_vmcb12_gpa;
>>>       /* These are the merged vectors */
>>>       u32 *msrpm;
>>> @@ -247,6 +248,11 @@ static inline void vmcb_mark_dirty(struct vmcb 
>>> *vmcb, int bit)
>>>       vmcb->control.clean &= ~(1 << bit);
>>>   }
>>> +static inline bool vmcb_is_dirty(struct vmcb *vmcb, int bit)
>>> +{
>>> +        return !test_bit(bit, (unsigned long *)&vmcb->control.clean);
>>> +}
>>> +
>>>   static inline struct vcpu_svm *to_svm(struct kvm_vcpu *vcpu)
>>>   {
>>>       return container_of(vcpu, struct vcpu_svm, vcpu);
>>> -- 
>>> 2.26.2
>>>
>