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

[Brijesh Singh]

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