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

[Andrei Semenov <andrei.semenov@xxxxxxxxxx>]

Thank you Jan for your feedback.

From what I understood, Andrew is planning some "ground" changes

on CPUID leaf discovery, so it probably will move here.  I'll wait for his

changes to apply your and Andrew's remarks and to re-post the patches.

Andrei.

On 4/18/24 10:13, Jan Beulich wrote:
> 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