| 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 |
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"
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" >
Il mer 20 mar 2024, 23:33 Michael Roth <michael.roth@amd.com> ha scritto: > 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. > Yeah, you most likely have the gpa anyway in the kind of board code that calls UPDATE_DATA. It would put some challenges with migration to use gpas everywhere instead of ram_addrs, but that doesn't use UPDATE_DATA and there's no SEV/SEV-ES migration anyway. > > > > > 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. > I would say especially in HMP do not bother since you have to start from a QAPI union and not QOM objects. For QMP I am not sure but don't waste too much effort in it. All I wanted to say is that the monitor is a fine place to draw the line. Paolo > 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 --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"