| Message ID | 20210826222627.3556-5-michael.roth@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) support | expand |
Hi Michael, On 27/08/2021 1:26, Michael Roth wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize > the platform. The command checks whether SNP is enabled in the KVM, if > enabled then it allocates a new ASID from the SNP pool and calls the > firmware to initialize the all the resources. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > target/i386/sev-stub.c | 6 ++++++ > target/i386/sev.c | 27 ++++++++++++++++++++++++--- > target/i386/sev_i386.h | 1 + > 3 files changed, 31 insertions(+), 3 deletions(-) > > diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c > index 0227cb5177..e4fb8e882e 100644 > --- a/target/i386/sev-stub.c > +++ b/target/i386/sev-stub.c > @@ -81,3 +81,9 @@ sev_get_attestation_report(const char *mnonce, Error **errp) > error_setg(errp, "SEV is not available in this QEMU"); > return NULL; > } > + > +bool > +sev_snp_enabled(void) > +{ > + return false; > +} > diff --git a/target/i386/sev.c b/target/i386/sev.c > index ba08b7d3ab..b8bd6ed9ea 100644 > --- a/target/i386/sev.c > +++ b/target/i386/sev.c > @@ -614,12 +614,21 @@ sev_enabled(void) > return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON); > } > > +bool > +sev_snp_enabled(void) > +{ > + ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; > + > + return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_SNP_GUEST); > +} > + > bool > sev_es_enabled(void) > { > ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; > > - return sev_enabled() && (SEV_GUEST(cgs)->policy & SEV_POLICY_ES); > + return sev_snp_enabled() || > + (sev_enabled() && SEV_GUEST(cgs)->policy & SEV_POLICY_ES); > } > > uint64_t > @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > uint32_t ebx; > uint32_t host_cbitpos; > struct sev_user_data_status status = {}; > + void *init_args = NULL; > > if (!sev_common) { > return 0; > @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > sev_common->api_major = status.api_major; > sev_common->api_minor = status.api_minor; Not visible here in the context: the code here is using the SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor. I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a struct sev_data_snp_platform_status (hmmm, I can't find the struct's definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI document). My questions are: 1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init an SNP guest? 2. Do we want to save some info like installed TCB version and reported TCB version, and maybe other fields from SNP platform status? 3. Should we check the state field in the platform status? > > - if (sev_es_enabled()) { > + if (sev_snp_enabled()) { > + SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common); > + if (!kvm_kernel_irqchip_allowed()) { > + error_report("%s: SEV-SNP guests require in-kernel irqchip support", > + __func__); Most errors in this function use error_setg(errp, ...). This should follow. > + goto err; > + } > + > + cmd = KVM_SEV_SNP_INIT; > + init_args = (void *)&sev_snp_guest->kvm_init_conf; > + > + } else if (sev_es_enabled()) { > if (!kvm_kernel_irqchip_allowed()) { > error_report("%s: SEV-ES guests require in-kernel irqchip support", > __func__); Not part of this patch, but this error_report (and another one in the SEV-ES case) should be converted to error_setg similarly. Maybe add a separate patch for fixing this for SEV-ES. > @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) > } > > trace_kvm_sev_init(); Suggestions: 1. log the guest type (SEV / SEV-ES / SEV-SNP) 2. log the SNP init flags value when initializing an SNP guest -Dov > - ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error); > + ret = sev_ioctl(sev_common->sev_fd, cmd, init_args, &fw_error); > if (ret) { > error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'", > __func__, ret, fw_error, fw_error_to_str(fw_error)); > diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h > index ae6d840478..e0e1a599be 100644 > --- a/target/i386/sev_i386.h > +++ b/target/i386/sev_i386.h > @@ -29,6 +29,7 @@ > #define SEV_POLICY_SEV 0x20 > > extern bool sev_es_enabled(void); > +extern bool sev_snp_enabled(void); > extern uint64_t sev_get_me_mask(void); > extern SevInfo *sev_get_info(void); > extern uint32_t sev_get_cbit_position(void); >
On 27/08/2021 1:26, Michael Roth wrote: > From: Brijesh Singh <brijesh.singh@amd.com> > > When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize > the platform. The command checks whether SNP is enabled in the KVM, if > enabled then it allocates a new ASID from the SNP pool and calls the > firmware to initialize the all the resources. > From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not* allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START. If that is indeed the case, I suggest removing this sentence here and adding it in the appropriate QEMU step (patch 5?). -Dov > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > Signed-off-by: Michael Roth <michael.roth@amd.com> > --- > target/i386/sev-stub.c | 6 ++++++ > target/i386/sev.c | 27 ++++++++++++++++++++++++--- > target/i386/sev_i386.h | 1 + > 3 files changed, 31 insertions(+), 3 deletions(-) > [...]
Hi Dov, On 9/5/21 2:07 AM, Dov Murik wrote: ... > >> >> uint64_t >> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) >> uint32_t ebx; >> uint32_t host_cbitpos; >> struct sev_user_data_status status = {}; >> + void *init_args = NULL; >> >> if (!sev_common) { >> return 0; >> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) >> sev_common->api_major = status.api_major; >> sev_common->api_minor = status.api_minor; > Not visible here in the context: the code here is using the > SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor. > > I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a > struct sev_data_snp_platform_status (hmmm, I can't find the struct's > definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI > document). The API version can be queries either through the SNP_PLATFORM_STATUS or SEV_PLATFORM_STATUS and they both report the same info. As the definition of the sev_data_platform_status is concerned it should be defined in the kernel include/linux/psp-sev.h. > My questions are: > > 1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init > an SNP guest? Yes, the legacy platform status command can be called on the SNP initialized host. I choose not to new command because we only care about the verison string and that is available through either of these commands (SNP or SEV platform status). > 2. Do we want to save some info like installed TCB version and reported > TCB version, and maybe other fields from SNP platform status? If we decide to add a new QMP (query-sev-snp) then it makes sense to export those fields so that a hypervisor console can give additional information; But note that for the guest, all these are available in the attestation report. > 3. Should we check the state field in the platform status? > > Good point, we could use the SNP platform status. I don't expect the state to be different between the SNP platform_status and SEV platform_status. >> >> - if (sev_es_enabled()) { >> + if (sev_snp_enabled()) { >> + SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common); >> + if (!kvm_kernel_irqchip_allowed()) { >> + error_report("%s: SEV-SNP guests require in-kernel irqchip support", >> + __func__); > Most errors in this function use error_setg(errp, ...). This should follow. > > >> + goto err; >> + } >> + >> + cmd = KVM_SEV_SNP_INIT; >> + init_args = (void *)&sev_snp_guest->kvm_init_conf; >> + >> + } else if (sev_es_enabled()) { >> if (!kvm_kernel_irqchip_allowed()) { >> error_report("%s: SEV-ES guests require in-kernel irqchip support", >> __func__); > Not part of this patch, but this error_report (and another one in the > SEV-ES case) should be converted to error_setg similarly. Maybe add a > separate patch for fixing this for SEV-ES. > > > >> @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) >> } >> >> trace_kvm_sev_init(); > Suggestions: > > 1. log the guest type (SEV / SEV-ES / SEV-SNP) > 2. log the SNP init flags value when initializing an SNP guest Noted. thanks
On 9/5/21 4:19 AM, Dov Murik wrote: > > On 27/08/2021 1:26, Michael Roth wrote: >> From: Brijesh Singh <brijesh.singh@amd.com> >> >> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize >> the platform. The command checks whether SNP is enabled in the KVM, if >> enabled then it allocates a new ASID from the SNP pool and calls the >> firmware to initialize the all the resources. >> > > From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add > KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not* > allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START. Actually, the KVM_SNP_INIT does allocate the ASID. If you look at the driver code then in switch state, the SNP_INIT fallthrough to SEV_INIT which will call sev_guest_init(). The sev_guest_init() allocates a new ASID. https://github.com/AMDESE/linux/blob/bb9ba49cd9b749d5551aae295c091d8757153dd7/arch/x86/kvm/svm/sev.c#L255 The LAUNCH_START simply binds the ASID to a guest. thanks
On 05/09/2021 17:05, Brijesh Singh wrote: > > On 9/5/21 4:19 AM, Dov Murik wrote: >> >> On 27/08/2021 1:26, Michael Roth wrote: >>> From: Brijesh Singh <brijesh.singh@amd.com> >>> >>> When SEV-SNP is enabled, the KVM_SNP_INIT command is used to initialize >>> the platform. The command checks whether SNP is enabled in the KVM, if >>> enabled then it allocates a new ASID from the SNP pool and calls the >>> firmware to initialize the all the resources. >>> >> >> From the KVM code ("[PATCH Part2 v5 24/45] KVM: SVM: Add >> KVM_SEV_SNP_LAUNCH_START command") it seems that KVM_SNP_INIT does *not* >> allocate the ASID; actually this is done in KVM_SEV_SNP_LAUNCH_START. > > Actually, the KVM_SNP_INIT does allocate the ASID. If you look at the > driver code then in switch state, the SNP_INIT fallthrough to SEV_INIT > which will call sev_guest_init(). The sev_guest_init() allocates a new > ASID. > https://github.com/AMDESE/linux/blob/bb9ba49cd9b749d5551aae295c091d8757153dd7/arch/x86/kvm/svm/sev.c#L255 > > The LAUNCH_START simply binds the ASID to a guest. OK thank you for clearing this up. So the kernel is choosing the new ASID during the KVM_SNP_INIT ioctl, but doesn't "tell" the firmware about it. Then later in SNP_LAUNCH_START that integer (saved in the kernel sev structure) is given to the firmware as an argument of the SNP_LAUNCH_START (binding?). Is this description correct? -Dov
On 05/09/2021 16:58, Brijesh Singh wrote: > Hi Dov, > > On 9/5/21 2:07 AM, Dov Murik wrote: > ... >> >>> >>> uint64_t >>> @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) >>> uint32_t ebx; >>> uint32_t host_cbitpos; >>> struct sev_user_data_status status = {}; >>> + void *init_args = NULL; >>> >>> if (!sev_common) { >>> return 0; >>> @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) >>> sev_common->api_major = status.api_major; >>> sev_common->api_minor = status.api_minor; >> Not visible here in the context: the code here is using the >> SEV_PLATFORM_STATUS command to get the build_id, api_major, and api_minor. >> >> I see that SNP has a new command SNP_PLATFORM_STATUS, which fills a >> struct sev_data_snp_platform_status (hmmm, I can't find the struct's >> definition; I assume it should look like Table 38 in 8.3.2 in SNP FW ABI >> document). > > The API version can be queries either through the SNP_PLATFORM_STATUS or > SEV_PLATFORM_STATUS and they both report the same info. As the > definition of the sev_data_platform_status is concerned it should be > defined in the kernel include/linux/psp-sev.h. > > >> My questions are: >> >> 1. Is it OK to call the "legacy" SEV_PLATFORM_STATUS when about to init >> an SNP guest? > > Yes, the legacy platform status command can be called on the SNP > initialized host. > > I choose not to new command because we only care about the verison > string and that is available through either of these commands (SNP or > SEV platform status). > >> 2. Do we want to save some info like installed TCB version and reported >> TCB version, and maybe other fields from SNP platform status? > > If we decide to add a new QMP (query-sev-snp) then it makes sense to > export those fields so that a hypervisor console can give additional > information; But note that for the guest, all these are available in the > attestation report. > We have new QMP response for SNP guests (SevSnpGuestProperties, patch 3 in this series). I think it would make sense to add the installed+reported TCB versions there (read-only properties), for debugging/observability purposes. -Dov
diff --git a/target/i386/sev-stub.c b/target/i386/sev-stub.c index 0227cb5177..e4fb8e882e 100644 --- a/target/i386/sev-stub.c +++ b/target/i386/sev-stub.c @@ -81,3 +81,9 @@ sev_get_attestation_report(const char *mnonce, Error **errp) error_setg(errp, "SEV is not available in this QEMU"); return NULL; } + +bool +sev_snp_enabled(void) +{ + return false; +} diff --git a/target/i386/sev.c b/target/i386/sev.c index ba08b7d3ab..b8bd6ed9ea 100644 --- a/target/i386/sev.c +++ b/target/i386/sev.c @@ -614,12 +614,21 @@ sev_enabled(void) return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_COMMON); } +bool +sev_snp_enabled(void) +{ + ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; + + return !!object_dynamic_cast(OBJECT(cgs), TYPE_SEV_SNP_GUEST); +} + bool sev_es_enabled(void) { ConfidentialGuestSupport *cgs = MACHINE(qdev_get_machine())->cgs; - return sev_enabled() && (SEV_GUEST(cgs)->policy & SEV_POLICY_ES); + return sev_snp_enabled() || + (sev_enabled() && SEV_GUEST(cgs)->policy & SEV_POLICY_ES); } uint64_t @@ -1074,6 +1083,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) uint32_t ebx; uint32_t host_cbitpos; struct sev_user_data_status status = {}; + void *init_args = NULL; if (!sev_common) { return 0; @@ -1126,7 +1136,18 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) sev_common->api_major = status.api_major; sev_common->api_minor = status.api_minor; - if (sev_es_enabled()) { + if (sev_snp_enabled()) { + SevSnpGuestState *sev_snp_guest = SEV_SNP_GUEST(sev_common); + if (!kvm_kernel_irqchip_allowed()) { + error_report("%s: SEV-SNP guests require in-kernel irqchip support", + __func__); + goto err; + } + + cmd = KVM_SEV_SNP_INIT; + init_args = (void *)&sev_snp_guest->kvm_init_conf; + + } else if (sev_es_enabled()) { if (!kvm_kernel_irqchip_allowed()) { error_report("%s: SEV-ES guests require in-kernel irqchip support", __func__); @@ -1145,7 +1166,7 @@ int sev_kvm_init(ConfidentialGuestSupport *cgs, Error **errp) } trace_kvm_sev_init(); - ret = sev_ioctl(sev_common->sev_fd, cmd, NULL, &fw_error); + ret = sev_ioctl(sev_common->sev_fd, cmd, init_args, &fw_error); if (ret) { error_setg(errp, "%s: failed to initialize ret=%d fw_error=%d '%s'", __func__, ret, fw_error, fw_error_to_str(fw_error)); diff --git a/target/i386/sev_i386.h b/target/i386/sev_i386.h index ae6d840478..e0e1a599be 100644 --- a/target/i386/sev_i386.h +++ b/target/i386/sev_i386.h @@ -29,6 +29,7 @@ #define SEV_POLICY_SEV 0x20 extern bool sev_es_enabled(void); +extern bool sev_snp_enabled(void); extern uint64_t sev_get_me_mask(void); extern SevInfo *sev_get_info(void); extern uint32_t sev_get_cbit_position(void);