[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v1 1/2] Implemented AMD SEV discovery and enabling.
On 4/11/24 20:32, Andrew Cooper wrote:
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.
Agree, will fix.
+
+ cpuid_count(0x80000000,0,&eax, &ebx, &ecx, &edx);
+
+ if (eax < 0x8000001f)
+ return;
Max leaf is already collected. c->extended_cpuid_level
Agree, will fix.
+
+ 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.
OK. I wait for your insights on this so.
+ }
+
+ 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?
Well, "normally it is". But this is the part of CPUID leaf specs. Do
they plan to potentially change it?
No idea.
+ }
+
+ 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?
Basically I put CONFIG_HVM here because I also wanted to put related
variables
(max/min_asid) in sev.c. And sev.c is in "HVM" part of the code as SEV
is only related to HVM guests. Now, basically I agree that
- Xen would like potentially use encrypted memory for itself
- in SME case, some encryption could be offered for non-HVM guests, so they
can protect their memory (even though the key is shared and the
hypervisor can
read it).
OK, so I will drop CONFIG_HVM and put these variables elsewhere. amd.h
is probably
a good candidate?
}
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.
Got it. Will do.
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.
Will do.
All these variables look like they should be __ro_after_init. However,
it's rather hard to judge, given no users yet.
Yes, this is not supposed to dynamically change. Will fix.
pte_c_bit_mask may want to be an intpte_t rather than uint64_t.
Agree. Will fix
~Andrew
Andrei.
|
