diff mbox series

[v3,37/49] i386/sev: Add the SNP launch start context

Message ID 20240320083945.991426-38-michael.roth@amd.com (mailing list archive)
State New, archived
Headers show
Series Add AMD Secure Nested Paging (SEV-SNP) support | expand

Commit Message

Michael Roth March 20, 2024, 8:39 a.m. UTC
From: Brijesh Singh <brijesh.singh@amd.com>

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>
Signed-off-by: Michael Roth <michael.roth@amd.com>
---
 target/i386/sev.c        | 42 +++++++++++++++++++++++++++++++++++++++-
 target/i386/trace-events |  1 +
 2 files changed, 42 insertions(+), 1 deletion(-)

Comments

Paolo Bonzini March 20, 2024, 9:58 a.m. UTC | #1
On 3/20/24 09:39, Michael Roth wrote:
> From: Brijesh Singh <brijesh.singh@amd.com>
> 
> 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>
> Signed-off-by: Michael Roth <michael.roth@amd.com>
> ---
>   target/i386/sev.c        | 42 +++++++++++++++++++++++++++++++++++++++-
>   target/i386/trace-events |  1 +
>   2 files changed, 42 insertions(+), 1 deletion(-)
> 
> diff --git a/target/i386/sev.c b/target/i386/sev.c
> index 3b4dbc63b1..9f63a41f08 100644
> --- a/target/i386/sev.c
> +++ b/target/i386/sev.c
> @@ -39,6 +39,7 @@
>   #include "confidential-guest.h"
>   #include "hw/i386/pc.h"
>   #include "exec/address-spaces.h"
> +#include "qemu/queue.h"
>   
>   OBJECT_DECLARE_SIMPLE_TYPE(SevCommonState, SEV_COMMON)
>   OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
> @@ -106,6 +107,16 @@ struct SevSnpGuestState {
>   #define DEFAULT_SEV_DEVICE      "/dev/sev"
>   #define DEFAULT_SEV_SNP_POLICY  0x30000
>   
> +typedef struct SevLaunchUpdateData {
> +    QTAILQ_ENTRY(SevLaunchUpdateData) next;
> +    hwaddr gpa;
> +    void *hva;
> +    uint64_t len;
> +    int type;
> +} SevLaunchUpdateData;
> +
> +static QTAILQ_HEAD(, SevLaunchUpdateData) launch_update;
> +
>   #define SEV_INFO_BLOCK_GUID     "00f771de-1a7e-4fcb-890e-68c77e2fb44e"
>   typedef struct __attribute__((__packed__)) SevInfoBlock {
>       /* SEV-ES Reset Vector Address */
> @@ -668,6 +679,30 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
>       return 0;
>   }
>   
> +static int
> +sev_snp_launch_start(SevSnpGuestState *sev_snp_guest)
> +{
> +    int fw_error, rc;
> +    SevCommonState *sev_common = SEV_COMMON(sev_snp_guest);
> +    struct kvm_sev_snp_launch_start *start = &sev_snp_guest->kvm_start_conf;
> +
> +    trace_kvm_sev_snp_launch_start(start->policy, sev_snp_guest->guest_visible_workarounds);
> +
> +    rc = sev_ioctl(sev_common->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__, rc, fw_error, fw_error_to_str(fw_error));
> +        return 1;
> +    }
> +
> +    QTAILQ_INIT(&launch_update);
> +
> +    sev_set_guest_state(sev_common, SEV_STATE_LAUNCH_UPDATE);
> +
> +    return 0;
> +}
> +
>   static int
>   sev_launch_start(SevGuestState *sev_guest)
>   {
> @@ -1007,7 +1042,12 @@ static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
>           goto err;
>       }
>   
> -    ret = sev_launch_start(SEV_GUEST(sev_common));
> +    if (sev_snp_enabled()) {
> +        ret = sev_snp_launch_start(SEV_SNP_GUEST(sev_common));
> +    } else {
> +        ret = sev_launch_start(SEV_GUEST(sev_common));
> +    }

Instead of an "if", this should be a method in sev-common.  Likewise for 
launch_finish in the next patch.

Also, patch 47 should introduce an "int (*launch_update_data)(hwaddr 
gpa, uint8_t *ptr, uint64_t len)" method whose implementation is either 
the existing sev_launch_update_data() for sev-guest, or a wrapper around 
snp_launch_update_data() (to add KVM_SEV_SNP_PAGE_TYPE_NORMAL) for 
sev-snp-guest.

In general, the only uses of sev_snp_enabled() should be in 
sev_add_kernel_loader_hashes() and kvm_handle_vmgexit_ext_req().  I 
would not be that strict for the QMP and HMP functions, but if you want 
to make those methods of sev-common I wouldn't complain.

Paolo

>       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..cb26d8a925 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, char *gosvw) "policy 0x%" PRIx64 " gosvw %s"
Michael Roth March 20, 2024, 10:32 p.m. UTC | #2
On Wed, Mar 20, 2024 at 10:58:30AM +0100, Paolo Bonzini wrote:
> On 3/20/24 09:39, Michael Roth wrote:
> > From: Brijesh Singh <brijesh.singh@amd.com>
> > 
> > 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>
> > Signed-off-by: Michael Roth <michael.roth@amd.com>
> > ---
> >   target/i386/sev.c        | 42 +++++++++++++++++++++++++++++++++++++++-
> >   target/i386/trace-events |  1 +
> >   2 files changed, 42 insertions(+), 1 deletion(-)
> > 
> > diff --git a/target/i386/sev.c b/target/i386/sev.c
> > index 3b4dbc63b1..9f63a41f08 100644
> > --- a/target/i386/sev.c
> > +++ b/target/i386/sev.c
> > @@ -39,6 +39,7 @@
> >   #include "confidential-guest.h"
> >   #include "hw/i386/pc.h"
> >   #include "exec/address-spaces.h"
> > +#include "qemu/queue.h"
> >   OBJECT_DECLARE_SIMPLE_TYPE(SevCommonState, SEV_COMMON)
> >   OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
> > @@ -106,6 +107,16 @@ struct SevSnpGuestState {
> >   #define DEFAULT_SEV_DEVICE      "/dev/sev"
> >   #define DEFAULT_SEV_SNP_POLICY  0x30000
> > +typedef struct SevLaunchUpdateData {
> > +    QTAILQ_ENTRY(SevLaunchUpdateData) next;
> > +    hwaddr gpa;
> > +    void *hva;
> > +    uint64_t len;
> > +    int type;
> > +} SevLaunchUpdateData;
> > +
> > +static QTAILQ_HEAD(, SevLaunchUpdateData) launch_update;
> > +
> >   #define SEV_INFO_BLOCK_GUID     "00f771de-1a7e-4fcb-890e-68c77e2fb44e"
> >   typedef struct __attribute__((__packed__)) SevInfoBlock {
> >       /* SEV-ES Reset Vector Address */
> > @@ -668,6 +679,30 @@ sev_read_file_base64(const char *filename, guchar **data, gsize *len)
> >       return 0;
> >   }
> > +static int
> > +sev_snp_launch_start(SevSnpGuestState *sev_snp_guest)
> > +{
> > +    int fw_error, rc;
> > +    SevCommonState *sev_common = SEV_COMMON(sev_snp_guest);
> > +    struct kvm_sev_snp_launch_start *start = &sev_snp_guest->kvm_start_conf;
> > +
> > +    trace_kvm_sev_snp_launch_start(start->policy, sev_snp_guest->guest_visible_workarounds);
> > +
> > +    rc = sev_ioctl(sev_common->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__, rc, fw_error, fw_error_to_str(fw_error));
> > +        return 1;
> > +    }
> > +
> > +    QTAILQ_INIT(&launch_update);
> > +
> > +    sev_set_guest_state(sev_common, SEV_STATE_LAUNCH_UPDATE);
> > +
> > +    return 0;
> > +}
> > +
> >   static int
> >   sev_launch_start(SevGuestState *sev_guest)
> >   {
> > @@ -1007,7 +1042,12 @@ static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
> >           goto err;
> >       }
> > -    ret = sev_launch_start(SEV_GUEST(sev_common));
> > +    if (sev_snp_enabled()) {
> > +        ret = sev_snp_launch_start(SEV_SNP_GUEST(sev_common));
> > +    } else {
> > +        ret = sev_launch_start(SEV_GUEST(sev_common));
> > +    }
> 
> Instead of an "if", this should be a method in sev-common.  Likewise for
> launch_finish in the next patch.

Makes sense.

> 
> Also, patch 47 should introduce an "int (*launch_update_data)(hwaddr gpa,
> uint8_t *ptr, uint64_t len)" method whose implementation is either the
> existing sev_launch_update_data() for sev-guest, or a wrapper around
> snp_launch_update_data() (to add KVM_SEV_SNP_PAGE_TYPE_NORMAL) for
> sev-snp-guest.

I suppose if we end up introducing an unused 'gpa' parameter in the case
of sev_launch_update_data() that's still worth the change? Seems
reasonable to me.

> 
> In general, the only uses of sev_snp_enabled() should be in
> sev_add_kernel_loader_hashes() and kvm_handle_vmgexit_ext_req().  I would
> not be that strict for the QMP and HMP functions, but if you want to make
> those methods of sev-common I wouldn't complain.

There's a good bit of duplication in those cases which is a little
awkward to break out into a common helper. Will consider these as well
though.

Thanks,

Mike

> 
> Paolo
> 
> >       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..cb26d8a925 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, char *gosvw) "policy 0x%" PRIx64 " gosvw %s"
>
diff mbox series

Patch

diff --git a/target/i386/sev.c b/target/i386/sev.c
index 3b4dbc63b1..9f63a41f08 100644
--- a/target/i386/sev.c
+++ b/target/i386/sev.c
@@ -39,6 +39,7 @@ 
 #include "confidential-guest.h"
 #include "hw/i386/pc.h"
 #include "exec/address-spaces.h"
+#include "qemu/queue.h"
 
 OBJECT_DECLARE_SIMPLE_TYPE(SevCommonState, SEV_COMMON)
 OBJECT_DECLARE_SIMPLE_TYPE(SevGuestState, SEV_GUEST)
@@ -106,6 +107,16 @@  struct SevSnpGuestState {
 #define DEFAULT_SEV_DEVICE      "/dev/sev"
 #define DEFAULT_SEV_SNP_POLICY  0x30000
 
+typedef struct SevLaunchUpdateData {
+    QTAILQ_ENTRY(SevLaunchUpdateData) next;
+    hwaddr gpa;
+    void *hva;
+    uint64_t len;
+    int type;
+} SevLaunchUpdateData;
+
+static QTAILQ_HEAD(, SevLaunchUpdateData) launch_update;
+
 #define SEV_INFO_BLOCK_GUID     "00f771de-1a7e-4fcb-890e-68c77e2fb44e"
 typedef struct __attribute__((__packed__)) SevInfoBlock {
     /* SEV-ES Reset Vector Address */
@@ -668,6 +679,30 @@  sev_read_file_base64(const char *filename, guchar **data, gsize *len)
     return 0;
 }
 
+static int
+sev_snp_launch_start(SevSnpGuestState *sev_snp_guest)
+{
+    int fw_error, rc;
+    SevCommonState *sev_common = SEV_COMMON(sev_snp_guest);
+    struct kvm_sev_snp_launch_start *start = &sev_snp_guest->kvm_start_conf;
+
+    trace_kvm_sev_snp_launch_start(start->policy, sev_snp_guest->guest_visible_workarounds);
+
+    rc = sev_ioctl(sev_common->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__, rc, fw_error, fw_error_to_str(fw_error));
+        return 1;
+    }
+
+    QTAILQ_INIT(&launch_update);
+
+    sev_set_guest_state(sev_common, SEV_STATE_LAUNCH_UPDATE);
+
+    return 0;
+}
+
 static int
 sev_launch_start(SevGuestState *sev_guest)
 {
@@ -1007,7 +1042,12 @@  static int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp)
         goto err;
     }
 
-    ret = sev_launch_start(SEV_GUEST(sev_common));
+    if (sev_snp_enabled()) {
+        ret = sev_snp_launch_start(SEV_SNP_GUEST(sev_common));
+    } else {
+        ret = sev_launch_start(SEV_GUEST(sev_common));
+    }
+
     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..cb26d8a925 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, char *gosvw) "policy 0x%" PRIx64 " gosvw %s"