| Message ID | 20210709215550.32496-5-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) support | expand |
Hi Brijesh, On 10/07/2021 0:55, Brijesh Singh wrote: > The SNP_LAUNCH_START is called first to create a cryptographic launch > context within the firmware. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > target/i386/sev.c | 30 +++++++++++++++++++++++++++++- > target/i386/trace-events | 1 + > 2 files changed, 30 insertions(+), 1 deletion(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 84ae244af0..259408a8f1 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -812,6 +812,29 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len) > return 0; > } > > +static int > +sev_snp_launch_start(SevGuestState *sev) > +{ > + int ret = 1; > + int fw_error, rc; > + struct kvm_sev_snp_launch_start *start = &sev->snp_config.start; > + > + trace_kvm_sev_snp_launch_start(start->policy); > + > + rc = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_START, start, &fw_error); > + if (rc < 0) { > + error_report("%s: SNP_LAUNCH_START ret=%d fw_error=%d '%s'", > + __func__, ret, fw_error, fw_error_to_str(fw_error)); Did you mean to report the value of ret or rc? > + goto out; Suggestion: Remove the `ret` variable. Here: simply `return 1`. At the end: remove the `out:` label; simply `return 0`. > + } > + > + sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE); > + ret = 0; > + > +out: > + return ret; > +} > + > static int > sev_launch_start(SevGuestState *sev) > { > @@ -1105,7 +1128,12 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > goto err; > } > > - ret = sev_launch_start(sev); > + if (sev_snp_enabled()) { > + ret = sev_snp_launch_start(sev); > + } else { > + ret = sev_launch_start(sev); > + } > + > if (ret) { > error_setg(errp, "%s: failed to create encryption context", __func__); > goto err; > diff --git a/target/i386/trace-events b/target/i386/trace-events > index 2cd8726eeb..18cc14b956 100644 > --- a/target/i386/trace-events > +++ b/target/i386/trace-events > @@ -11,3 +11,4 @@ kvm_sev_launch_measurement(const char *value) "data %s" > kvm_sev_launch_finish(void) "" > kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d" > kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s" > +kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64 >
On 7/19/21 7:34 AM, Dov Murik wrote: > Hi Brijesh, > > On 10/07/2021 0:55, Brijesh Singh wrote: >> The SNP_LAUNCH_START is called first to create a cryptographic launch >> context within the firmware. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> target/i386/sev.c | 30 +++++++++++++++++++++++++++++- >> target/i386/trace-events | 1 + >> 2 files changed, 30 insertions(+), 1 deletion(-) >> >> diff --git a/target/i386/sev.c b/target/i386/sev.c >> index 84ae244af0..259408a8f1 100644 >> --- a/target/i386/sev.c >> +++ b/target/i386/sev.c >> @@ -812,6 +812,29 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len) >> return 0; >> } >> >> +static int >> +sev_snp_launch_start(SevGuestState *sev) >> +{ >> + int ret = 1; >> + int fw_error, rc; >> + struct kvm_sev_snp_launch_start *start = &sev->snp_config.start; >> + >> + trace_kvm_sev_snp_launch_start(start->policy); >> + >> + rc = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_START, start, &fw_error); >> + if (rc < 0) { >> + error_report("%s: SNP_LAUNCH_START ret=%d fw_error=%d '%s'", >> + __func__, ret, fw_error, fw_error_to_str(fw_error)); > > Did you mean to report the value of ret or rc? Ah, I was meaning the rc. > > >> + goto out; > > Suggestion: > > Remove the `ret` variable. > Here: simply `return 1`. > At the end: remove the `out:` label; simply `return 0`. > Noted. thanks
diff --git a/target/i386/sev.c b/target/i386/sev.c index 84ae244af0..259408a8f1 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -812,6 +812,29 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len) return 0; } +static int +sev_snp_launch_start(SevGuestState *sev) +{ + int ret = 1; + int fw_error, rc; + struct kvm_sev_snp_launch_start *start = &sev->snp_config.start; + + trace_kvm_sev_snp_launch_start(start->policy); + + rc = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_START, start, &fw_error); + if (rc < 0) { + error_report("%s: SNP_LAUNCH_START ret=%d fw_error=%d '%s'", + __func__, ret, fw_error, fw_error_to_str(fw_error)); + goto out; + } + + sev_set_guest_state(sev, SEV_STATE_LAUNCH_UPDATE); + ret = 0; + +out: + return ret; +} + static int sev_launch_start(SevGuestState *sev) { @@ -1105,7 +1128,12 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) goto err; } - ret = sev_launch_start(sev); + if (sev_snp_enabled()) { + ret = sev_snp_launch_start(sev); + } else { + ret = sev_launch_start(sev); + } + if (ret) { error_setg(errp, "%s: failed to create encryption context", __func__); goto err; diff --git a/target/i386/trace-events b/target/i386/trace-events index 2cd8726eeb..18cc14b956 100644 --- a/target/i386/trace-events +++ b/target/i386/trace-events @@ -11,3 +11,4 @@ kvm_sev_launch_measurement(const char *value) "data %s" kvm_sev_launch_finish(void) "" kvm_sev_launch_secret(uint64_t hpa, uint64_t hva, uint64_t secret, int len) "hpa 0x%" PRIx64 " hva 0x%" PRIx64 " data 0x%" PRIx64 " len %d" kvm_sev_attestation_report(const char *mnonce, const char *data) "mnonce %s data %s" +kvm_sev_snp_launch_start(uint64_t policy) "policy 0x%" PRIx64
The SNP_LAUNCH_START is called first to create a cryptographic launch context within the firmware. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- target/i386/sev.c | 30 +++++++++++++++++++++++++++++- target/i386/trace-events | 1 + 2 files changed, 30 insertions(+), 1 deletion(-)