1. Patchew
  2. QEMU
  3. [RFC PATCH 0/6] Add AMD Secure Nested Paging (SEV-SNP) support
  4. View patch

[RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled

Brijesh Singh posted 6 patches 4 years, 7 months ago
Maintainers: "Daniel P. Berrangé" <berrange@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>, Paolo Bonzini <pbonzini@redhat.com>, "Michael S. Tsirkin" <mst@redhat.com>, Cornelia Huck <cohuck@redhat.com>, Markus Armbruster <armbru@redhat.com>, Eric Blake <eblake@redhat.com>
There is a newer version of this series
[RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
Posted by Brijesh Singh 4 years, 7 months ago 
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;
+    }
+
+    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"
-- 
2.17.1
Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
Posted by Connor Kuehl 4 years, 6 months ago 
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
Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
Posted by Brijesh Singh 4 years, 6 months ago 

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
Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
Posted by Dov Murik 4 years, 6 months ago 

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
Re: [RFC PATCH 5/6] i386/sev: add support to encrypt BIOS when SEV-SNP is enabled
Posted by Dov Murik 4 years, 6 months ago 

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"
>

Patchew 2.1-devel-b67e4c2

© 2016 - 2026 Red Hat, Inc.