Re: [PATCH v5 2/7] nSVM: introduce smv->nested.save to cache save area fields

[Maxim Levitsky <mlevitsk@redhat.com>]

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@redhat.com>
> ---
>  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@redhat.com>

Best regards,
	Maxim Levitsky