| Message ID | 20210709215550.32496-6-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) support | expand |
On 7/9/21 3:55 PM, Brijesh Singh wrote: > The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios > image used for booting the SEV-SNP guest. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > target/i386/sev.c | 33 ++++++++++++++++++++++++++++++++- > target/i386/trace-events | 1 + > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 259408a8f1..41dcb084d1 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -883,6 +883,30 @@ out: > return ret; > } > > +static int > +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type) > +{ > + int ret, fw_error; > + struct kvm_sev_snp_launch_update update = {}; > + > + if (!addr || !len) { > + return 1; Should this be a -1? It looks like the caller checks if this function returns < 0, but doesn't check for res == 1. Alternatively, invoking error_report might provide more useful information that the preconditions to this function were violated. Connor
On 7/14/21 12:08 PM, Connor Kuehl wrote: > On 7/9/21 3:55 PM, Brijesh Singh wrote: >> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios >> image used for booting the SEV-SNP guest. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> target/i386/sev.c | 33 ++++++++++++++++++++++++++++++++- >> target/i386/trace-events | 1 + >> 2 files changed, 33 insertions(+), 1 deletion(-) >> >> diff --git a/target/i386/sev.c b/target/i386/sev.c >> index 259408a8f1..41dcb084d1 100644 >> --- a/target/i386/sev.c >> +++ b/target/i386/sev.c >> @@ -883,6 +883,30 @@ out: >> return ret; >> } >> >> +static int >> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type) >> +{ >> + int ret, fw_error; >> + struct kvm_sev_snp_launch_update update = {}; >> + >> + if (!addr || !len) { >> + return 1; > > Should this be a -1? It looks like the caller checks if this function > returns < 0, but doesn't check for res == 1. Ah, it should be -1. > > Alternatively, invoking error_report might provide more useful > information that the preconditions to this function were violated. > Sure, I will add error_report. thanks
On 14/07/2021 21:52, Brijesh Singh wrote: > > > On 7/14/21 12:08 PM, Connor Kuehl wrote: >> On 7/9/21 3:55 PM, Brijesh Singh wrote: >>> The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios >>> image used for booting the SEV-SNP guest. >>> >>> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >>> --- >>> target/i386/sev.c | 33 ++++++++++++++++++++++++++++++++- >>> target/i386/trace-events | 1 + >>> 2 files changed, 33 insertions(+), 1 deletion(-) >>> >>> diff --git a/target/i386/sev.c b/target/i386/sev.c >>> index 259408a8f1..41dcb084d1 100644 >>> --- a/target/i386/sev.c >>> +++ b/target/i386/sev.c >>> @@ -883,6 +883,30 @@ out: >>> return ret; >>> } >>> +static int >>> +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t >>> len, int type) >>> +{ >>> + int ret, fw_error; >>> + struct kvm_sev_snp_launch_update update = {}; >>> + >>> + if (!addr || !len) { >>> + return 1; >> >> Should this be a -1? It looks like the caller checks if this function >> returns < 0, but doesn't check for res == 1. > > Ah, it should be -1. > >> >> Alternatively, invoking error_report might provide more useful >> information that the preconditions to this function were violated. >> > > Sure, I will add error_report. Maybe even simpler: assert(addr); assert(len > 0); The assertion failure will show the developer what is wrong. This should not happen for the end-user (unless I'm missing something). -Dov
On 10/07/2021 0:55, Brijesh Singh wrote: > The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios > image used for booting the SEV-SNP guest. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > target/i386/sev.c | 33 ++++++++++++++++++++++++++++++++- > target/i386/trace-events | 1 + > 2 files changed, 33 insertions(+), 1 deletion(-) > > diff --git a/target/i386/sev.c b/target/i386/sev.c > index 259408a8f1..41dcb084d1 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -883,6 +883,30 @@ out: > return ret; > } > > +static int > +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type) This seems similar to the SEV LAUNCH_UPDATE_DATA API (with the added `type` argument). In SEV API these are the limitations (from the SEV API document): * PADDR - System physical address of the data to be encrypted. Must be 16 B aligned. * LENGTH - Length of the data to be encrypted. Must be a multiple of 16 B. But in SNP_LAUNCH_UPDATE it is my understanding that addr must be page aligned (4KB) and length must be in whole pages (because the underlying types are PAGE_TYPE_NORMAL, PAGE_TYPE_ZERO, ...). So what happens if we call sev_encrypt_flash with a non-page-aligned addr / length? Or maybe I misunderstood the SNP ABI document? -Dov > +{ > + int ret, fw_error; > + struct kvm_sev_snp_launch_update update = {}; > + > + if (!addr || !len) { > + return 1; > + } > + > + update.uaddr = (__u64)(unsigned long)addr; > + update.len = len; > + update.page_type = type; > + trace_kvm_sev_snp_launch_update(addr, len, type); > + ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE, > + &update, &fw_error); > + if (ret) { > + error_report("%s: SNP_LAUNCH_UPDATE ret=%d fw_error=%d '%s'", > + __func__, ret, fw_error, fw_error_to_str(fw_error)); > + } > + > + return ret; > +} > + > static int > sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len) > { > @@ -1161,7 +1185,14 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp) > > /* if SEV is in update state then encrypt the data else do nothing */ > if (sev_check_state(sev_guest, SEV_STATE_LAUNCH_UPDATE)) { > - int ret = sev_launch_update_data(sev_guest, ptr, len); > + int ret; > + > + if (sev_snp_enabled()) { > + ret = sev_snp_launch_update(sev_guest, ptr, len, > + KVM_SEV_SNP_PAGE_TYPE_NORMAL); > + } else { > + ret = sev_launch_update_data(sev_guest, ptr, len); > + } > if (ret < 0) { > error_setg(errp, "failed to encrypt pflash rom"); > return ret; > diff --git a/target/i386/trace-events b/target/i386/trace-events > index 18cc14b956..0c2d250206 100644 > --- a/target/i386/trace-events > +++ b/target/i386/trace-events > @@ -12,3 +12,4 @@ 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 > +kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d" >
diff --git a/target/i386/sev.c b/target/i386/sev.c index 259408a8f1..41dcb084d1 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -883,6 +883,30 @@ out: return ret; } +static int +sev_snp_launch_update(SevGuestState *sev, uint8_t *addr, uint64_t len, int type) +{ + int ret, fw_error; + struct kvm_sev_snp_launch_update update = {}; + + if (!addr || !len) { + return 1; + } + + update.uaddr = (__u64)(unsigned long)addr; + update.len = len; + update.page_type = type; + trace_kvm_sev_snp_launch_update(addr, len, type); + ret = sev_ioctl(sev->sev_fd, KVM_SEV_SNP_LAUNCH_UPDATE, + &update, &fw_error); + if (ret) { + error_report("%s: SNP_LAUNCH_UPDATE ret=%d fw_error=%d '%s'", + __func__, ret, fw_error, fw_error_to_str(fw_error)); + } + + return ret; +} + static int sev_launch_update_data(SevGuestState *sev, uint8_t *addr, uint64_t len) { @@ -1161,7 +1185,14 @@ sev_encrypt_flash(uint8_t *ptr, uint64_t len, Error **errp) /* if SEV is in update state then encrypt the data else do nothing */ if (sev_check_state(sev_guest, SEV_STATE_LAUNCH_UPDATE)) { - int ret = sev_launch_update_data(sev_guest, ptr, len); + int ret; + + if (sev_snp_enabled()) { + ret = sev_snp_launch_update(sev_guest, ptr, len, + KVM_SEV_SNP_PAGE_TYPE_NORMAL); + } else { + ret = sev_launch_update_data(sev_guest, ptr, len); + } if (ret < 0) { error_setg(errp, "failed to encrypt pflash rom"); return ret; diff --git a/target/i386/trace-events b/target/i386/trace-events index 18cc14b956..0c2d250206 100644 --- a/target/i386/trace-events +++ b/target/i386/trace-events @@ -12,3 +12,4 @@ 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 +kvm_sev_snp_launch_update(void *addr, uint64_t len, int type) "addr %p len 0x%" PRIx64 " type %d"
The KVM_SEV_SNP_LAUNCH_UPDATE command is used for encrypting the bios image used for booting the SEV-SNP guest. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- target/i386/sev.c | 33 ++++++++++++++++++++++++++++++++- target/i386/trace-events | 1 + 2 files changed, 33 insertions(+), 1 deletion(-)