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
