Re: [RFC PATCH 6/6] i386/sev: populate secrets and cpuid page and finalize the SNP launch

Brijesh Singh
2021-07-19T14:45:46Z

Hi Dov,

On 7/19/21 6:24 AM, Dov Murik wrote:
>
> s/LAUNCH_UPDATE/SNP_LAUNCH_UPDATE/
> (to show it's the same command you refer to above)
>

Noted.

>>
>> +static int
>> +sev_snp_launch_update_gpa(uint32_t hwaddr, uint32_t size, uint8_t type)
>
> hwaddr is a confusing name here because it is also a typedef (which is
> most likely uint64_t...). Maybe call this argument `gpa` ?
>

Noted, 'gpa' sounds much better.

>> +static bool
>> +detectoverlap(uint32_t start, uint32_t end,
>> + struct snp_pre_validated_range *overlap)
>
> naming conventions dictate: detect_overlap
>

Noted.

>> +{
>> + int i;
>> +
>> + for (i = 0; i < ARRAY_SIZE(pre_validated); i++) {
>> + if (pre_validated[i].start < end && start < pre_validated[i].end) {
>> + memcpy(overlap, &pre_validated[i], sizeof(*overlap));
>
> Maybe simpler than memcpy:
>
> *overlap = pre_validated[i];
>

Noted.

>> +
>> + trace_kvm_sev_snp_launch_finish();
>
> Maybe the trace should show some info about the snp_config.finish fields?
>

I did thought about it, but one of the field in the snp_config.finish is
4K in size and may fill the trace buffer quickly.

>> +kvm_sev_snp_ovmf_boot_block_info(uint32_t secrets_gpa, uint32_t slen, uint32_t cpuid_gpa, uint32_t clen, uint32_t s, uint32_t e) "secrets 0x%x+0x%x cpuid 0x%x+0x%x pre-validate 0x%x+0x%x"
>
> The last argument is an end-addr (not a length), so maybe the format
> string should end with:
>
> ".... pre-validate 0x%x - 0x%x"
>
> Also I'd prefer to log the SevSnpBootInfoBlock fields in the order they
> appear in the struct.
>
>

Noted.

thanks