| Message ID | 20210818053908.1907051-1-mizhang@google.com |
|---|---|
| Headers | show |
| Series | clean up interface between KVM and psp | expand |
On Tue, Aug 17, 2021 at 10:39 PM Mingwei Zhang <mizhang@google.com> wrote: > > sev_decommission is needed in the error path of sev_bind_asid. The purpose > of this function is to clear the firmware context. Missing this step may > cause subsequent SEV launch failures. > > Although missing sev_decommission issue has previously been found and was > fixed in sev_launch_start function. It is supposed to be fixed on all > scenarios where a firmware context needs to be freed. According to the AMD > SEV API v0.24 Section 1.3.3: > > "The RECEIVE_START command is the only command other than the LAUNCH_START > command that generates a new guest context and guest handle." > > The above indicates that RECEIVE_START command also requires calling > sev_decommission if ASID binding fails after RECEIVE_START succeeds. > > So add the sev_decommission function in sev_receive_start. > > Cc: Alper Gun <alpergun@google.com> > Cc: Borislav Petkov <bp@alien8.de> > Cc: Brijesh Singh <brijesh.singh@amd.com> > Cc: David Rienjes <rientjes@google.com> > Cc: Marc Orr <marcorr@google.com> > Cc: John Allen <john.allen@amd.com> > Cc: Peter Gonda <pgonda@google.com> > Cc: Sean Christopherson <seanjc@google.com> > Cc: Tom Lendacky <thomas.lendacky@amd.com> > Cc: Vipin Sharma <vipinsh@google.com> > > Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command") > Signed-off-by: Mingwei Zhang <mizhang@google.com> > --- > arch/x86/kvm/svm/sev.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > index 75e0b21ad07c..55d8b9c933c3 100644 > --- a/arch/x86/kvm/svm/sev.c > +++ b/arch/x86/kvm/svm/sev.c > @@ -1397,8 +1397,10 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > > /* Bind ASID to this guest */ > ret = sev_bind_asid(kvm, start.handle, error); > - if (ret) > + if (ret) { > + sev_decommission(start.handle); > goto e_free_session; > + } > > params.handle = start.handle; > if (copy_to_user((void __user *)(uintptr_t)argp->data, > -- > 2.33.0.rc1.237.g0d66db33f3-goog Should this patch have the following Fixes tag? Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command") Reviewed-by: Marc Orr <marcorr@google.com>
On Fri, Aug 20, 2021 at 7:11 PM Marc Orr <marcorr@google.com> wrote: > > On Tue, Aug 17, 2021 at 10:39 PM Mingwei Zhang <mizhang@google.com> wrote: > > > > sev_decommission is needed in the error path of sev_bind_asid. The purpose > > of this function is to clear the firmware context. Missing this step may > > cause subsequent SEV launch failures. > > > > Although missing sev_decommission issue has previously been found and was > > fixed in sev_launch_start function. It is supposed to be fixed on all > > scenarios where a firmware context needs to be freed. According to the AMD > > SEV API v0.24 Section 1.3.3: > > > > "The RECEIVE_START command is the only command other than the LAUNCH_START > > command that generates a new guest context and guest handle." > > > > The above indicates that RECEIVE_START command also requires calling > > sev_decommission if ASID binding fails after RECEIVE_START succeeds. > > > > So add the sev_decommission function in sev_receive_start. > > > > Cc: Alper Gun <alpergun@google.com> > > Cc: Borislav Petkov <bp@alien8.de> > > Cc: Brijesh Singh <brijesh.singh@amd.com> > > Cc: David Rienjes <rientjes@google.com> > > Cc: Marc Orr <marcorr@google.com> > > Cc: John Allen <john.allen@amd.com> > > Cc: Peter Gonda <pgonda@google.com> > > Cc: Sean Christopherson <seanjc@google.com> > > Cc: Tom Lendacky <thomas.lendacky@amd.com> > > Cc: Vipin Sharma <vipinsh@google.com> > > > > Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command") > > Signed-off-by: Mingwei Zhang <mizhang@google.com> > > --- > > arch/x86/kvm/svm/sev.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c > > index 75e0b21ad07c..55d8b9c933c3 100644 > > --- a/arch/x86/kvm/svm/sev.c > > +++ b/arch/x86/kvm/svm/sev.c > > @@ -1397,8 +1397,10 @@ static int sev_receive_start(struct kvm *kvm, struct kvm_sev_cmd *argp) > > > > /* Bind ASID to this guest */ > > ret = sev_bind_asid(kvm, start.handle, error); > > - if (ret) > > + if (ret) { > > + sev_decommission(start.handle); > > goto e_free_session; > > + } > > > > params.handle = start.handle; > > if (copy_to_user((void __user *)(uintptr_t)argp->data, > > -- > > 2.33.0.rc1.237.g0d66db33f3-goog > > Should this patch have the following Fixes tag? > > Fixes: af43cbbf954b ("KVM: SVM: Add support for KVM_SEV_RECEIVE_START command") Oops. I missed that it already has the Fixes tag. Please ignore this comment.
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, ¶ms, 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(¶ms, (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, ¶ms.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, ¶ms, sizeof(struct kvm_sev_receive_start))) return -EFAULT; sev->handle = params.handle; sev->fd = argp->sev_fd; return 0; }
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, > ¶ms, 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(¶ms, (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, > ¶ms.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, > ¶ms, 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
On Tue, Sep 07, 2021, Brijesh Singh wrote: > > On 9/3/21 2:38 PM, Sean Christopherson wrote: > > 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(¶ms, (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, > > ¶ms.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, > > ¶ms, 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. Yeah, that's brutal, but IMO having a few ugly functions is an acceptable cost if it means the rest of the API is cleaner. E.g. KVM is not the right place to implement sev_deactivate_lock, as any coincident DEACTIVATE will be problematic. The current code "works" because KVM is the only in-tree user, but even that's a bit of a grey area because sev_guest_deactivate() is exported. If large param lists are problematic, one idea would be to reuse the sev_data_* structs for the API. I still don't like the idea of exposing those structs outside of the PSP driver, and the potential user vs. kernel pointer confusion is more than a bit ugly. On the other hand it's not exactly secret info, e.g. KVM's UAPI structs are already excrutiatingly close to sev_data_* structs. For future ioctls(), KVM could even define UAPI structs that are bit-for-bit compatible with the hardware structs. That would allow KVM to copy userspace's data directly into a "struct sev_data_*" and simply require the handle and any other KVM-defined params to be zero. KVM could then hand the whole struct over to the PSP driver for processing. We can even do a direct copy to sev_data* with KVM's current UAPI by swapping fields as necessary, e.g. swap policy<->handle before and after send-start, but that's all kinds of gross and probably not a net positive.
On 9/7/21 6:37 PM, Sean Christopherson wrote: > On Tue, Sep 07, 2021, Brijesh Singh wrote: >> >> On 9/3/21 2:38 PM, Sean Christopherson wrote: >>> 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(¶ms, (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, >>> ¶ms.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, >>> ¶ms, 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. > > Yeah, that's brutal, but IMO having a few ugly functions is an acceptable cost if > it means the rest of the API is cleaner. E.g. KVM is not the right place to > implement sev_deactivate_lock, as any coincident DEACTIVATE will be problematic. > The current code "works" because KVM is the only in-tree user, but even that's a > bit of a grey area because sev_guest_deactivate() is exported. > > If large param lists are problematic, one idea would be to reuse the sev_data_* > structs for the API. I still don't like the idea of exposing those structs > outside of the PSP driver, and the potential user vs. kernel pointer confusion > is more than a bit ugly. On the other hand it's not exactly secret info, > e.g. KVM's UAPI structs are already excrutiatingly close to sev_data_* structs. > > For future ioctls(), KVM could even define UAPI structs that are bit-for-bit > compatible with the hardware structs. That would allow KVM to copy userspace's > data directly into a "struct sev_data_*" and simply require the handle and any > other KVM-defined params to be zero. KVM could then hand the whole struct over > to the PSP driver for processing. Most of the address field in the "struct sev_data_*" are physical addressess. The userspace will not be able to populate those fields. PSP or KVM may still need to assist filling the final hardware structure. Some of fields in hardware structure must be zero, so we need to add checks for it. I can try posting RFC post SNP series and we can see how it all looks. > > We can even do a direct copy to sev_data* with KVM's current UAPI by swapping > fields as necessary, e.g. swap policy<->handle before and after send-start, but > that's all kinds of gross and probably not a net positive. >
On Thu, Sep 09, 2021, Brijesh Singh wrote: > > On 9/7/21 6:37 PM, Sean Christopherson wrote: > > On Tue, Sep 07, 2021, Brijesh Singh wrote: > > > 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. > > > > Yeah, that's brutal, but IMO having a few ugly functions is an acceptable cost if > > it means the rest of the API is cleaner. E.g. KVM is not the right place to > > implement sev_deactivate_lock, as any coincident DEACTIVATE will be problematic. > > The current code "works" because KVM is the only in-tree user, but even that's a > > bit of a grey area because sev_guest_deactivate() is exported. > > > > If large param lists are problematic, one idea would be to reuse the sev_data_* > > structs for the API. I still don't like the idea of exposing those structs > > outside of the PSP driver, and the potential user vs. kernel pointer confusion > > is more than a bit ugly. On the other hand it's not exactly secret info, > > e.g. KVM's UAPI structs are already excrutiatingly close to sev_data_* structs. > > > > For future ioctls(), KVM could even define UAPI structs that are bit-for-bit > > compatible with the hardware structs. That would allow KVM to copy userspace's > > data directly into a "struct sev_data_*" and simply require the handle and any > > other KVM-defined params to be zero. KVM could then hand the whole struct over > > to the PSP driver for processing. > > Most of the address field in the "struct sev_data_*" are physical > addressess. The userspace will not be able to populate those fields. Yeah, that's my biggest hesitation to using struct sev_data_* in the API, it's both confusing and gross. But it's also why I think these helpers belong in the PSP driver, KVM should not need to know the "on-the-wire" format for communicating with the PSP. > PSP or KVM may still need to assist filling the final hardware structure. > Some of fields in hardware structure must be zero, so we need to add checks > for it. > I can try posting RFC post SNP series and we can see how it all looks. I'm a bit torn. I completely understand the desire to get SNP support merged, but at the same time KVM has accrued a fair bit of technical debt for SEV and SEV-ES, and the lack of tests is also a concern. I don't exactly love the idea of kicking those cans further down the road. Paolo, any thoughts?
> > Most of the address field in the "struct sev_data_*" are physical > > addressess. The userspace will not be able to populate those fields. > > Yeah, that's my biggest hesitation to using struct sev_data_* in the API, it's > both confusing and gross. But it's also why I think these helpers belong in the > PSP driver, KVM should not need to know the "on-the-wire" format for communicating > with the PSP. > Did a simple checking for all struct sev_data_* fields defined in psp-sev.h: The average argument is roughly 4 (103/27), detailed data appended at last. In addition, I believe the most used commands would be the following? #data structure name: number of meaningful fields sev_data_launch_start: 6 sev_data_activate: 2 sev_data_decommission: 1 sev_data_receive_update_data: 7 sev_data_send_update_vmsa: 7 sev_data_launch_measure: 3 sev_data_launch_finish: 1 sev_data_deactivate: 1 For the above frequently-used command set, the average argument length is also around 3-4 (28/8) on average, 2.5 as the median. So, from that perspective, I think we should just remove those sev_data data structures in KVM, since it is more clear to read each argument. In addition, having to construct each sev_data_* structure in KVM code is also a pain and consumes a lot of irrelevant lines as well. #data structure name: number of meaningful fields sev_data_deactivate: 1 sev_data_decommission: 1 sev_data_launch_finish: 1 sev_data_receive_finish: 1 sev_data_send_cancel: 1 sev_data_send_finish: 1 sev_data_activate: 2 sev_data_download_firmware: 2 sev_data_get_id: 2 sev_data_pek_csr: 2 sev_data_init: 3 sev_data_launch_measure: 3 sev_data_launch_update_data: 3 sev_data_launch_update_vmsa: 3 sev_data_attestation_report: 4 sev_data_dbg: 4 sev_data_guest_status: 4 sev_data_pdh_cert_export: 4 sev_data_pek_cert_import: 4 sev_data_launch_start: 6 sev_data_receive_start: 6 sev_data_launch_secret: 7 sev_data_receive_update_data: 7 sev_data_receive_update_vmsa: 7 sev_data_send_update_data: 7 sev_data_send_update_vmsa: 7 sev_data_send_start: 10
On 9/9/21 4:18 PM, Mingwei Zhang wrote: >>> Most of the address field in the "struct sev_data_*" are physical >>> addressess. The userspace will not be able to populate those fields. >> >> Yeah, that's my biggest hesitation to using struct sev_data_* in the API, it's >> both confusing and gross. But it's also why I think these helpers belong in the >> PSP driver, KVM should not need to know the "on-the-wire" format for communicating >> with the PSP. >> > > Did a simple checking for all struct sev_data_* fields defined in psp-sev.h: thank you for compiling the list, let me add few more at the end of your list (they are part of spec but PSP/KVM does not have support for it). I am also adding SNP specific so that we get a full picture. > > The average argument is roughly 4 (103/27), detailed data appended at > last. In addition, I believe the most used commands would be the > following? > > #data structure name: number of meaningful fields > sev_data_launch_start: 6 > sev_data_activate: 2 > sev_data_decommission: 1 > sev_data_receive_update_data: 7 > sev_data_send_update_vmsa: 7 > sev_data_launch_measure: 3 > sev_data_launch_finish: 1 > sev_data_deactivate: 1 > Once the page copy, and swap in/out is implemented then it will also be frequently used. sev_data_copy: 4 sev_swap_out: 12 sev_swap_in: 7 snp_data_gctx_create: 1 snp_data_activate: 2 snp_data_deactivate: 2 snp_data_decommission: 1 snp_data_launch_start: 6 snp_data_launch_update: 8 snp_data_launch_finish: 6 snp_data_page_move: 4 snp_data_page_swap_out: 8 snp_data_page_swap_in: 8 snp_data_page_reclaim: 2 snp_guest_request: 3 snp_guest_request_ext: 5 > For the above frequently-used command set, the average argument length > is also around 3-4 (28/8) on average, 2.5 as the median. > It averages around 4-5 (51/11) without snp. The good news is with snp the avg is still 4-5 (107/24). Additionally, we also need to pass the sev->fd in each of these functions, which will increase avg to 5-6. > So, from that perspective, I think we should just remove those > sev_data data structures in KVM, since it is more clear to read each > argument. > I believe once we are done with it, will have 5 functions that will need >=8 arguments. I don't know if its acceptable. > In addition, having to construct each sev_data_* structure in KVM code > is also a pain and consumes a lot of irrelevant lines as well. > Maybe I am missing something, aren't those lines will be moved from KVM to PSP driver? I am in full support for restructuring, but lets look at full set of PSP APIs before making the final decision. thanks > #data structure name: number of meaningful fields > sev_data_deactivate: 1 > sev_data_decommission: 1 > sev_data_launch_finish: 1 > sev_data_receive_finish: 1 > sev_data_send_cancel: 1 > sev_data_send_finish: 1 > sev_data_activate: 2 > sev_data_download_firmware: 2 > sev_data_get_id: 2 > sev_data_pek_csr: 2 > sev_data_init: 3 > sev_data_launch_measure: 3 > sev_data_launch_update_data: 3 > sev_data_launch_update_vmsa: 3 > sev_data_attestation_report: 4 > sev_data_dbg: 4 > sev_data_guest_status: 4 > sev_data_pdh_cert_export: 4 > sev_data_pek_cert_import: 4 > sev_data_launch_start: 6 > sev_data_receive_start: 6 > sev_data_launch_secret: 7 > sev_data_receive_update_data: 7 > sev_data_receive_update_vmsa: 7 > sev_data_send_update_data: 7 > sev_data_send_update_vmsa: 7 > sev_data_send_start: 10 >
> I believe once we are done with it, will have 5 functions that will need > >=8 arguments. I don't know if its acceptable. > > > In addition, having to construct each sev_data_* structure in KVM code > > is also a pain and consumes a lot of irrelevant lines as well. > > > > Maybe I am missing something, aren't those lines will be moved from KVM > to PSP driver? > > I am in full support for restructuring, but lets look at full set of PSP > APIs before making the final decision. > > thanks > Oh, sorry for the confusion. I think the current feedback I got is that my restructuring patchset was blocked due to the fact that it is a partial one. So, if this patchset got checked in, then the psp-sev.h will have two types of APIs: ones that use sev_data_* structure and ones that do not. So one of the worries is that this would make the situation even worse. So that's why I am thinking that maybe it is fine to just avoid using sev_data_* for all PSP functions exposed to KVM? I use the number of arguments as the justification. But that might not be a good one. In anycase, I will not rush into any code change before we reach a consensus. Thanks. -Mingwei
On Thu, Sep 9, 2021 at 6:18 PM Mingwei Zhang <mizhang@google.com> wrote: > > > I believe once we are done with it, will have 5 functions that will need > > >=8 arguments. I don't know if its acceptable. > > > > > In addition, having to construct each sev_data_* structure in KVM code > > > is also a pain and consumes a lot of irrelevant lines as well. > > > > > > > Maybe I am missing something, aren't those lines will be moved from KVM > > to PSP driver? > > > > I am in full support for restructuring, but lets look at full set of PSP > > APIs before making the final decision. > > > > thanks > > > > Oh, sorry for the confusion. I think the current feedback I got is > that my restructuring patchset was blocked due to the fact that it is > a partial one. So, if this patchset got checked in, then the psp-sev.h > will have two types of APIs: ones that use sev_data_* structure and > ones that do not. So one of the worries is that this would make the > situation even worse. > > So that's why I am thinking that maybe it is fine to just avoid using > sev_data_* for all PSP functions exposed to KVM? I use the number of > arguments as the justification. But that might not be a good one. > > In anycase, I will not rush into any code change before we reach a consensus. Isn't the first patch in this patch set a straight-forward bug fix :-)? Assuming others agree, I'd suggest to re-send that one out as a single patch on its own, so we can get it merged while the rest of this patch set works its way through the process.