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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 10/04/2024 4:36 pm, Andrei Semenov wrote:
> diff --git a/xen/arch/x86/cpu/amd.c b/xen/arch/x86/cpu/amd.c
> index ab92333673..a5903613f0 100644
> --- a/xen/arch/x86/cpu/amd.c
> +++ b/xen/arch/x86/cpu/amd.c
> @@ -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()) {

c == &boot_cpu_info.

> +
> +             cpuid_count(0x80000000,0,&eax, &ebx, &ecx, &edx);
> +
> +             if (eax <  0x8000001f)
> +                     return;

Max leaf is already collected.  c->extended_cpuid_level


> +
> +             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);
> +                     max_sev_asid = ecx;
> +                     min_sev_asid = edx;
> +             }
> +
> +             if (eax & 0x3)
> +                     pte_c_bit_mask = 1UL << (ebx & 0x3f);

This is decoding the main SEV feature leaf, but outside of normal
mechanisms.

I've got half a mind to brute-force through the remaining work to
un-screw our boot sequence order, and express this in a cpu-policy
straight away.  This is wanted for the SVM leaf info too.

Leave it with me for a bit.


> +     }
> +
> +     if (!(cpu_has_sme || cpu_has_sev))
> +             return;
> +
> +     if (!smp_processor_id()) {
> +             if (cpu_has_sev)
> +                     printk(XENLOG_INFO "SEV: ASID range [0x%x - 0x%x]\n",
> +                     min_sev_asid, max_sev_asid);

Why do we have a min as well as a max?  Isn't min always 1?

> +     }
> +
> +     rdmsrl(MSR_K8_SYSCFG, syscfg);
> +
> +     if (syscfg & SYSCFG_MEM_ENCRYPT) {
> +             return;
> +     }
> +
> +     syscfg |= SYSCFG_MEM_ENCRYPT;
> +     wrmsrl(MSR_K8_SYSCFG, syscfg);
> +}
> +#endif
> +
>  static void cf_check init_amd(struct cpuinfo_x86 *c)
>  {
>       u32 l, h;
> @@ -1305,6 +1354,10 @@ static void cf_check init_amd(struct cpuinfo_x86 *c)
>       check_syscfg_dram_mod_en();
>  
>       amd_log_freq(c);
> +
> +#ifdef CONFIG_HVM
> +     amd_enable_mem_encrypt(c);
> +#endif

I think we want to drop the CONFIG_HVM here.

Memory encryption is an all-or-nothing thing.  If it's active, it
affects all pagetables that Xen controls, even dom0's.  And we likely do
want get to the point of Xen running on encrypted mappings even if dom0
can't operate it very nicely.

Thoughts?

>  }
>  
>  const struct cpu_dev __initconst_cf_clobber amd_cpu_dev = {
> diff --git a/xen/arch/x86/hvm/svm/Makefile b/xen/arch/x86/hvm/svm/Makefile
> index 760d2954da..9773d539ef 100644
> --- a/xen/arch/x86/hvm/svm/Makefile
> +++ b/xen/arch/x86/hvm/svm/Makefile
> @@ -6,3 +6,4 @@ obj-y += nestedsvm.o
>  obj-y += svm.o
>  obj-y += svmdebug.o
>  obj-y += vmcb.o
> +obj-y += sev.o

Please keep this sorted by object file name.

> diff --git a/xen/arch/x86/hvm/svm/sev.c b/xen/arch/x86/hvm/svm/sev.c
> new file mode 100644
> index 0000000000..336fad25f5
> --- /dev/null
> +++ b/xen/arch/x86/hvm/svm/sev.c
> @@ -0,0 +1,4 @@
> +#include <asm/sev.h>
> +uint64_t __read_mostly pte_c_bit_mask;
> +unsigned int __read_mostly min_sev_asid;
> +unsigned int __read_mostly max_sev_asid;

Several things.  All new files should come with an SPDX tag.  Unless you
have other constraints, GPL-2.0-only is preferred.  There also wants to
be at least a oneline summary of what's going on here.

All these variables look like they should be __ro_after_init.  However,
it's rather hard to judge, given no users yet.

pte_c_bit_mask may want to be an intpte_t rather than uint64_t.

~Andrew