Re: [PATCH v1 1/2] Implemented AMD SEV discovery and enabling.

[Jan Beulich <jbeulich@xxxxxxxx>]

On 10.04.2024 17:36, Andrei Semenov wrote:
> Signed-off-by: Andrei Semenov <andrei.semenov@xxxxxxxx>

A couple more nits on top of what Andrew said. First: Please no patches
(which aren't blindingly trivial) without description.

> @@ -1030,6 +1031,54 @@ static void amd_check_erratum_1485(void)
>       wrmsrl(MSR_AMD64_BP_CFG, val | chickenbit);
>  }
>  
> +#ifdef CONFIG_HVM
> +static void amd_enable_mem_encrypt(const struct cpuinfo_x86 *c)
> +{
> +     unsigned int  eax, ebx, ecx, edx;
> +     uint64_t syscfg;
> +
> +     if (!smp_processor_id()) {
> +
> +             cpuid_count(0x80000000,0,&eax, &ebx, &ecx, &edx);

No blank line above here please.

> +             if (eax <  0x8000001f)
> +                     return;
> +
> +             cpuid_count(0x8000001f,0,&eax, &ebx, &ecx, &edx);
> +
> +             if (eax & 0x1)
> +                     setup_force_cpu_cap(X86_FEATURE_SME);
> +
> +             if (eax & 0x2) {
> +                     setup_force_cpu_cap(X86_FEATURE_SEV);

I guess this goes along with what Andrew said: Using synthetic features here
looks suspicious. These want to be recorded as an ordinary leaf.

> +                     max_sev_asid = ecx;
> +                     min_sev_asid = edx;
> +             }
> +
> +             if (eax & 0x3)
> +                     pte_c_bit_mask = 1UL << (ebx & 0x3f);
> +     }
> +
> +     if (!(cpu_has_sme || cpu_has_sev))
> +             return;
> +
> +     if (!smp_processor_id()) {
> +             if (cpu_has_sev)

Two if()-s like these want folding, unless it is made clear that very
soon (see above as to the missing description) further content is going
to appear inside the outer one.

> +                     printk(XENLOG_INFO "SEV: ASID range [0x%x - 0x%x]\n",

%#x is preferred over 0x%x.

> +                     min_sev_asid, max_sev_asid);
> +     }
> +
> +     rdmsrl(MSR_K8_SYSCFG, syscfg);
> +
> +     if (syscfg & SYSCFG_MEM_ENCRYPT) {
> +             return;
> +     }

No need for braces in cases like this one.

Jan