On Wed, 2021-11-03 at 10:05 -0400, Emanuele Giuseppe Esposito wrote:
> This is useful in next patch, to avoid having temporary
> copies of vmcb12 registers and passing them manually.
>
> Right now, instead of blindly copying everything,
> we just copy EFER, CR0, CR3, CR4, DR6 and DR7. If more fields
> will need to be added, it will be more obvious to see
> that they must be added in struct vmcb_save_area_cached and
> in nested_copy_vmcb_save_to_cache().
>
> _nested_copy_vmcb_save_to_cache() takes a vmcb_save_area_cached
> parameter, useful when we want to save the state to a local
> variable instead of svm internals.
>
> Note that in svm_set_nested_state() we want to cache the L2
> save state only if we are in normal non guest mode, because
> otherwise it is not touched.
>
> Signed-off-by: Emanuele Giuseppe Esposito <eesposit@xxxxxxxxxx>
> ---
> arch/x86/kvm/svm/nested.c | 27 ++++++++++++++++++++++++++-
> arch/x86/kvm/svm/svm.c | 1 +
> arch/x86/kvm/svm/svm.h | 16 ++++++++++++++++
> 3 files changed, 43 insertions(+), 1 deletion(-)
>
> diff --git a/arch/x86/kvm/svm/nested.c b/arch/x86/kvm/svm/nested.c
> index 946c06a25d37..ea64fea371c8 100644
> --- a/arch/x86/kvm/svm/nested.c
> +++ b/arch/x86/kvm/svm/nested.c
> @@ -328,6 +328,28 @@ void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> svm->nested.ctl.iopm_base_pa &= ~0x0fffULL;
> }
>
> +static void _nested_copy_vmcb_save_to_cache(struct vmcb_save_area_cached *to,
> + struct vmcb_save_area *from)
> +{
> + /*
> + * Copy only fields that are validated, as we need them
> + * to avoid TOC/TOU races.
> + */
> + to->efer = from->efer;
> + to->cr0 = from->cr0;
> + to->cr3 = from->cr3;
> + to->cr4 = from->cr4;
> +
> + to->dr6 = from->dr6;
> + to->dr7 = from->dr7;
> +}
> +
> +void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
> + struct vmcb_save_area *save)
> +{
> + _nested_copy_vmcb_save_to_cache(&svm->nested.save, save);
> +}
> +
> /*
> * Synchronize fields that are written by the processor, so that
> * they can be copied back into the vmcb12.
> @@ -670,6 +692,7 @@ int nested_svm_vmrun(struct kvm_vcpu *vcpu)
> return -EINVAL;
>
> nested_load_control_from_vmcb12(svm, &vmcb12->control);
> + nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
>
> if (!nested_vmcb_valid_sregs(vcpu, &vmcb12->save) ||
> !nested_vmcb_check_controls(vcpu, &svm->nested.ctl)) {
> @@ -1402,8 +1425,10 @@ static int svm_set_nested_state(struct kvm_vcpu *vcpu,
>
> if (is_guest_mode(vcpu))
> svm_leave_nested(svm);
> - else
> + else {
> svm->nested.vmcb02.ptr->save = svm->vmcb01.ptr->save;
> + nested_copy_vmcb_save_to_cache(svm, &svm->nested.vmcb02.ptr->save);
> + }
>
> svm_set_gif(svm, !!(kvm_state->flags & KVM_STATE_NESTED_GIF_SET));
>
> diff --git a/arch/x86/kvm/svm/svm.c b/arch/x86/kvm/svm/svm.c
> index 21bb81710e0f..6e5c2671e823 100644
> --- a/arch/x86/kvm/svm/svm.c
> +++ b/arch/x86/kvm/svm/svm.c
> @@ -4438,6 +4438,7 @@ static int svm_leave_smm(struct kvm_vcpu *vcpu, const char *smstate)
>
> vmcb12 = map.hva;
> nested_load_control_from_vmcb12(svm, &vmcb12->control);
> + nested_copy_vmcb_save_to_cache(svm, &vmcb12->save);
> ret = enter_svm_guest_mode(vcpu, vmcb12_gpa, vmcb12, false);
>
> unmap_save:
> diff --git a/arch/x86/kvm/svm/svm.h b/arch/x86/kvm/svm/svm.h
> index 0d7bbe548ac3..0897551d8868 100644
> --- a/arch/x86/kvm/svm/svm.h
> +++ b/arch/x86/kvm/svm/svm.h
> @@ -103,6 +103,19 @@ struct kvm_vmcb_info {
> uint64_t asid_generation;
> };
>
Nitpick: There is a bit of ambiguity in this comment. I think you mean that
the contents of the svm->nested.save are not always up to date,
but then the comment should be there, where the field is
declared.
I first read this comment as if the fields of this struct are not
up to date vs SVM spec or something.
Also the same can be said about the svm->nested.ctl and such when not
running a nested guest. Maybe we need to drop that coment at all.
> +/*
> + * This struct is not kept up-to-date, and it is only valid within
> + * svm_set_nested_state and nested_svm_vmrun.
> + */
> +struct vmcb_save_area_cached {
> + u64 efer;
> + u64 cr4;
> + u64 cr3;
> + u64 cr0;
> + u64 dr7;
> + u64 dr6;
> +};
> +
> struct svm_nested_state {
> struct kvm_vmcb_info vmcb02;
> u64 hsave_msr;
> @@ -119,6 +132,7 @@ struct svm_nested_state {
>
> /* cache for control fields of the guest */
> struct vmcb_control_area ctl;
> + struct vmcb_save_area_cached save;
>
> bool initialized;
> };
> @@ -490,6 +504,8 @@ void nested_svm_update_tsc_ratio_msr(struct kvm_vcpu *vcpu);
> void svm_write_tsc_multiplier(struct kvm_vcpu *vcpu, u64 multiplier);
> void nested_load_control_from_vmcb12(struct vcpu_svm *svm,
> struct vmcb_control_area *control);
> +void nested_copy_vmcb_save_to_cache(struct vcpu_svm *svm,
> + struct vmcb_save_area *save);
> void nested_sync_control_from_vmcb02(struct vcpu_svm *svm);
> void nested_vmcb02_compute_g_pat(struct vcpu_svm *svm);
> void svm_switch_vmcb(struct vcpu_svm *svm, struct kvm_vmcb_info *target_vmcb);
Other than the nitpick looks good.
Reviewed-by: Maxim Levitsky <mlevitsk@xxxxxxxxxx>
Best regards,
Maxim Levitsky
