LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Brijesh Singh <brijesh.singh@amd.com>
To: Sean Christopherson <seanjc@google.com>,
	Mingwei Zhang <mizhang@google.com>
Cc: brijesh.singh@amd.com, Paolo Bonzini <pbonzini@redhat.com>,
	Tom Lendacky <thomas.lendacky@amd.com>,
	John Allen <john.allen@amd.com>,
	Vitaly Kuznetsov <vkuznets@redhat.com>,
	Wanpeng Li <wanpengli@tencent.com>,
	Jim Mattson <jmattson@google.com>, Joerg Roedel <joro@8bytes.org>,
	kvm@vger.kernel.org, linux-crypto@vger.kernel.org,
	linux-kernel@vger.kernel.org, Alper Gun <alpergun@google.com>,
	Borislav Petkov <bp@alien8.de>,
	David Rienjes <rientjes@google.com>,
	Marc Orr <marcorr@google.com>, Peter Gonda <pgonda@google.com>,
	Vipin Sharma <vipinsh@google.com>
Subject: Re: [PATCH v2 3/4] KVM: SVM: move sev_bind_asid to psp
Date: Tue, 7 Sep 2021 11:30:45 -0500	[thread overview]
Message-ID: <fcb83a85-8150-9617-01e6-c6bcc249c485@amd.com> (raw)
In-Reply-To: <YTJ5wjNShaHlDVAp@google.com>



On 9/3/21 2:38 PM, Sean Christopherson wrote:
> On Wed, Aug 18, 2021, Mingwei Zhang wrote:
>> @@ -336,11 +322,9 @@ static int sev_launch_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
>>   		goto e_free_session;
>>   
>>   	/* Bind ASID to this guest */
>> -	ret = sev_bind_asid(kvm, start.handle, error);
>> -	if (ret) {
>> -		sev_guest_decommission(start.handle, NULL);
>> +	ret = sev_guest_bind_asid(sev_get_asid(kvm), start.handle, error);
>> +	if (ret)
>>   		goto e_free_session;
>> -	}
>>   
>>   	/* return handle to userspace */
>>   	params.handle = start.handle;
> 
> ...
> 
>> diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c
>> index e2d49bedc0ef..325e79360d9e 100644
>> --- a/drivers/crypto/ccp/sev-dev.c
>> +++ b/drivers/crypto/ccp/sev-dev.c
>> @@ -903,6 +903,21 @@ int sev_guest_activate(struct sev_data_activate *data, int *error)
>>   }
>>   EXPORT_SYMBOL_GPL(sev_guest_activate);
>>   
>> +int sev_guest_bind_asid(int asid, unsigned int handle, int *error)
>> +{
>> +	struct sev_data_activate activate;
>> +	int ret;
>> +
>> +	/* activate ASID on the given handle */
>> +	activate.handle = handle;
>> +	activate.asid   = asid;
>> +	ret = sev_guest_activate(&activate, error);
>> +	if (ret)
>> +		sev_guest_decommission(handle, NULL);
> 
> Hrm, undoing state like this is a bad API.  It assumes the caller is well-behaved,
> e.g. has already done something that requires decommissioning, and it surprises
> the caller, e.g. the KVM side (above) looks like it's missing error handling.
> Something like this would be cleaner overall:
> 
> 	/* create memory encryption context */
> 	ret = __sev_issue_cmd(argp->sev_fd, SEV_CMD_RECEIVE_START, &start,
> 				error);
> 	if (ret)
> 		goto e_free_session;
> 
> 	/* Bind ASID to this guest */
> 	ret = sev_guest_activate(sev_get_asid(kvm), start.handle, error);
> 	if (ret)
> 		goto e_decommision;
> 
> 	params.handle = start.handle;
> 	if (copy_to_user((void __user *)(uintptr_t)argp->data,
> 			 &params, sizeof(struct kvm_sev_receive_start))) {
> 		ret = -EFAULT;
> 		goto e_deactivate;
> 	}
> 
>      	sev->handle = start.handle;
> 	sev->fd = argp->sev_fd;
> 
> e_deactivate:
> 	sev_guest_deactivate(sev_get_asid(kvm), start.handle, error);
> e_decommision:
> 	sev_guest_decommission(start.handle, error);
> e_free_session:
> 	kfree(session_data);
> e_free_pdh:
> 	kfree(pdh_data);
> 
> 
> However, I don't know that that's a good level of abstraction, e.g. the struct
> details are abstracted from KVM but the exact sequencing is not, which is odd
> to say the least.
> 
> Which is a good segue into my overarching complaint about the PSP API and what
> made me suggest this change in the first place.  IMO, the API exposed to KVM (and
> others) is too low level, e.g. KVM is practically making direct calls to the PSP
> via sev_issue_cmd_external_user().  Even the partially-abstracted helpers that
> take a "struct sev_data_*" are too low level, KVM really shouldn't need to know
> the hardware-defined structures for an off-CPU device.
> 
> My intent with the suggestion was to start driving toward a mostly-abstracted API
> across the board, with an end goal of eliminating sev_issue_cmd_external_user()
> and moving all of the sev_data* structs out of psp-sev.h and into a private
> header.  However, I think we should all explicitly agree on the desired level of
> abstraction before shuffling code around.
> 
> My personal preference is obviously to work towards an abstracted API.  And if
> we decide to go that route, I think we should be much more aggressive with respect
> to what is abstracted.   Many of the functions will be rather gross due to the
> sheer number of params, but I think the end result will be a net positive in terms
> of readability and separation of concerns.
> 
> E.g. get KVM looking like this
> 
> static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp)
> {
> 	struct kvm_sev_info *sev = &to_kvm_svm(kvm)->sev_info;
> 	struct kvm_sev_receive_start params;
> 	int ret;
> 
> 	if (!sev_guest(kvm))
> 		return -ENOTTY;
> 
> 	/* Get parameter from the userspace */
> 	if (copy_from_user(&params, (void __user *)(uintptr_t)argp->data,
> 			sizeof(struct kvm_sev_receive_start)))
> 		return -EFAULT;
> 
> 	ret = sev_guest_receive_start(argp->sev_fd, &arpg->error, sev->asid,
> 				      &params.handle, params.policy,
> 				      params.pdh_uaddr, params.pdh_len,
> 				      params.session_uaddr, params.session_len);
> 	if (ret)
> 		return ret;
> 
> 	/* Copy params back to user even on failure, e.g. for error info. */
> 	if (copy_to_user((void __user *)(uintptr_t)argp->data,
> 			 &params, sizeof(struct kvm_sev_receive_start)))
> 		return -EFAULT;
> 
>      	sev->handle = params.handle;
> 	sev->fd = argp->sev_fd;
> 	return 0;
> }
> 

I have no strong preference for either of the abstraction approaches. 
The sheer number of argument can also make some folks wonder whether 
such abstraction makes it easy to read. e.g send-start may need up to 11.

thanks

- Brijesh

  reply	other threads:[~2021-09-07 16:30 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-18  5:39 [PATCH v2 0/4] clean up interface between KVM and psp Mingwei Zhang
2021-08-18  5:39 ` [PATCH v2 1/4] KVM: SVM: fix missing sev_decommission in sev_receive_start Mingwei Zhang
2021-08-21  2:11   ` Marc Orr
2021-08-21  2:30     ` Marc Orr
2021-08-18  5:39 ` [PATCH v2 2/4] KVM: SVM: move sev_decommission to psp driver Mingwei Zhang
2021-08-18  5:39 ` [PATCH v2 3/4] KVM: SVM: move sev_bind_asid to psp Mingwei Zhang
2021-09-03 19:38   ` Sean Christopherson
2021-09-07 16:30     ` Brijesh Singh [this message]
2021-09-07 23:37       ` Sean Christopherson
2021-09-09 16:07         ` Brijesh Singh
2021-09-09 18:13           ` Sean Christopherson
2021-09-09 21:18             ` Mingwei Zhang
2021-09-09 22:25               ` Brijesh Singh
2021-09-10  1:18                 ` Mingwei Zhang
2021-09-10  1:23                   ` Marc Orr
2021-08-18  5:39 ` [PATCH v2 4/4] KVM: SVM: move sev_unbind_asid and DF_FLUSH logic into psp Mingwei Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=fcb83a85-8150-9617-01e6-c6bcc249c485@amd.com \
    --to=brijesh.singh@amd.com \
    --cc=alpergun@google.com \
    --cc=bp@alien8.de \
    --cc=jmattson@google.com \
    --cc=john.allen@amd.com \
    --cc=joro@8bytes.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=marcorr@google.com \
    --cc=mizhang@google.com \
    --cc=pbonzini@redhat.com \
    --cc=pgonda@google.com \
    --cc=rientjes@google.com \
    --cc=seanjc@google.com \
    --cc=thomas.lendacky@amd.com \
    --cc=vipinsh@google.com \
    --cc=vkuznets@redhat.com \
    --cc=wanpengli@tencent.com \
    --subject='Re: [PATCH v2 3/4] KVM: SVM: move sev_bind_asid to psp' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).