Re: [RFC PATCH v2 04/12] i386/sev: initialize SNP context

[Brijesh Singh <brijesh.singh@amd.com>]

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