linux-kernel - Re: [PATCH v4 5/9] KVM: nSVM: introduce nested_svm_load_cr3()/nested_npt

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: <87zh82s5gf.fsf@vitty.brq.redhat.com>
Date:   Tue, 14 Jul 2020 13:26:24 +0200
From:   Vitaly Kuznetsov <vkuznets@...hat.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     kvm@...r.kernel.org, Paolo Bonzini <pbonzini@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Junaid Shahid <junaids@...gle.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4 5/9] KVM: nSVM: introduce nested_svm_load_cr3()/nested_npt_enabled()

Sean Christopherson <sean.j.christopherson@...el.com> writes:

> On Fri, Jul 10, 2020 at 04:11:53PM +0200, Vitaly Kuznetsov wrote:
>> As a preparatory change for implementing nested specifig PGD switch for
>
> s/specifig/specific
>
>> nSVM (following nVMX' nested_vmx_load_cr3()) instead of relying on
>
> nVMX's
>
>> kvm_set_cr3() introduce nested_svm_load_cr3().
>
> The changelog isn't all that helpful to understanding the actual change.
> All this is doing is wrapping kvm_set_cr3(), but that's not at all obvious
> from reading the above.
>
> E.g.
>
>   Add nested_svm_load_cr3() as a pass-through to kvm_set_cr3() as a
>   preparatory change for implementing nested specific PGD switch for nSVM,
>   (following nVMx's nested_vmx_load_cr3()).
>

Sounds better indeed, thanks!

>> No functional change intended.
>> 
>> Signed-off-by: Vitaly Kuznetsov <vkuznets@...hat.com>
>> ---
>>  arch/x86/kvm/svm/nested.c | 21 +++++++++++++++++++--
>>  1 file changed, 19 insertions(+), 2 deletions(-)
>> 
>> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
>> index 5e6c988a4e6b..180929f3dbef 100644
>> --- a/arch/x86/kvm/svm/nested.c
>> +++ b/arch/x86/kvm/svm/nested.c
>> @@ -311,6 +311,21 @@ static void nested_vmcb_save_pending_event(struct vcpu_svm *svm,
>>  	nested_vmcb->control.exit_int_info = exit_int_info;
>>  }
>>  
>> +static inline bool nested_npt_enabled(struct vcpu_svm *svm)
>> +{
>> +	return svm->nested.ctl.nested_ctl & SVM_NESTED_CTL_NP_ENABLE;
>> +}
>> +
>> +/*
>> + * Load guest's cr3 at nested entry. @nested_npt is true if we are
>> + * emulating VM-Entry into a guest with NPT enabled.
>> + */
>> +static int nested_svm_load_cr3(struct kvm_vcpu *vcpu, unsigned long cr3,
>> +			       bool nested_npt)
>
> IMO the addition of nested_npt_enabled() should be a separate patch, and
> the additoin of @nested_npt should be in patch 7.
>
> Hypothetically speaking, if nested_npt_enabled() is inaccurate at the call
> site in nested_prepare_vmcb_save(), then this patch is technically wrong
> even though it doesn't introduce a bug.  Given that the call site of
> nested_svm_load_cr3() is moved in patch 7, I don't see any value in adding
> the placeholder parameter early.
>

I see and I mostly agree, I put it here to avoid the unneeded churn and
make it easier to review the whole thing: this patch is technically a
nop so it can be reviewed in "doesn't change anything" mode and patches
which actually change things are smaller.

Paolo already said 'queued' here and your comments can't be addressed in
a follow-up patch but I can certainly do v5 if needed.

Thanks for your review!

-- 
Vitaly