| Message ID | 20240327154317.29909-6-bp@alien8.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/sev: Fix SNP host late disable | expand |
On 27/03/2024 16:43, Borislav Petkov wrote: > From: "Borislav Petkov (AMD)" <bp@alien8.de> > > The host SNP worthiness can determined later, after alternatives have > been patched, in snp_rmptable_init() depending on cmdline options like > iommu=pt which is incompatible with SNP, for example. > > Which means that one cannot use X86_FEATURE_SEV_SNP and will need to > have a special flag for that control. > > Use that newly added CC_ATTR_HOST_SEV_SNP in the appropriate places. > > Move kdump_sev_callback() to its rightfull place, while at it. > > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> > --- > arch/x86/include/asm/sev.h | 4 ++-- > arch/x86/kernel/cpu/amd.c | 38 ++++++++++++++++++------------ > arch/x86/kernel/cpu/mtrr/generic.c | 2 +- > arch/x86/kernel/sev.c | 10 -------- > arch/x86/kvm/svm/sev.c | 2 +- > arch/x86/virt/svm/sev.c | 26 +++++++++++++------- > drivers/crypto/ccp/sev-dev.c | 2 +- > drivers/iommu/amd/init.c | 4 +++- > 8 files changed, 49 insertions(+), 39 deletions(-) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index 9477b4053bce..780182cda3ab 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -228,7 +228,6 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct sn > void snp_accept_memory(phys_addr_t start, phys_addr_t end); > u64 snp_get_unsupported_features(u64 status); > u64 sev_get_status(void); > -void kdump_sev_callback(void); > void sev_show_status(void); > #else > static inline void sev_es_ist_enter(struct pt_regs *regs) { } > @@ -258,7 +257,6 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in > static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { } > static inline u64 snp_get_unsupported_features(u64 status) { return 0; } > static inline u64 sev_get_status(void) { return 0; } > -static inline void kdump_sev_callback(void) { } > static inline void sev_show_status(void) { } > #endif > > @@ -270,6 +268,7 @@ int psmash(u64 pfn); > int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immutable); > int rmp_make_shared(u64 pfn, enum pg_level level); > void snp_leak_pages(u64 pfn, unsigned int npages); > +void kdump_sev_callback(void); > #else > static inline bool snp_probe_rmptable_info(void) { return false; } > static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; } > @@ -282,6 +281,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 as > } > static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; } > static inline void snp_leak_pages(u64 pfn, unsigned int npages) {} > +static inline void kdump_sev_callback(void) { } > #endif > > #endif > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index 6d8677e80ddb..9bf17c9c29da 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -345,6 +345,28 @@ static void srat_detect_node(struct cpuinfo_x86 *c) > #endif > } > > +static void bsp_determine_snp(struct cpuinfo_x86 *c) > +{ > +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM > + cc_vendor = CC_VENDOR_AMD; Shouldn't this line be inside the cpu_has(c, X86_FEATURE_SEV_SNP) check? > + > + if (cpu_has(c, X86_FEATURE_SEV_SNP)) { > + /* > + * RMP table entry format is not architectural and is defined by the > + * per-processor PPR. Restrict SNP support on the known CPU models > + * for which the RMP table entry format is currently defined for. > + */> + if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && How about turning this into a more specific check: if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && Thanks, Jeremi > + c->x86 >= 0x19 && snp_probe_rmptable_info()) { > + cc_platform_set(CC_ATTR_HOST_SEV_SNP); > + } else { > + setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); > + cc_platform_clear(CC_ATTR_HOST_SEV_SNP); > + } > + } > +#endif > +} > + > static void bsp_init_amd(struct cpuinfo_x86 *c) > { > if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) { > @@ -452,21 +474,7 @@ static void bsp_init_amd(struct cpuinfo_x86 *c) > break; > } > > - if (cpu_has(c, X86_FEATURE_SEV_SNP)) { > - /* > - * RMP table entry format is not architectural and it can vary by processor > - * and is defined by the per-processor PPR. Restrict SNP support on the > - * known CPU model and family for which the RMP table entry format is > - * currently defined for. > - */ > - if (!boot_cpu_has(X86_FEATURE_ZEN3) && > - !boot_cpu_has(X86_FEATURE_ZEN4) && > - !boot_cpu_has(X86_FEATURE_ZEN5)) > - setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); > - else if (!snp_probe_rmptable_info()) > - setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); > - } > - > + bsp_determine_snp(c); > return; > > warn:
On Thu, Mar 28, 2024 at 12:51:17PM +0100, Jeremi Piotrowski wrote: > Shouldn't this line be inside the cpu_has(c, X86_FEATURE_SEV_SNP) check? The cc_vendor is not dependent on X86_FEATURE_SEV_SNP. > How about turning this into a more specific check: > > if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && Why? The check is "am I running as a hypervisor on baremetal".
On 28/03/2024 14:41, Borislav Petkov wrote: > On Thu, Mar 28, 2024 at 12:51:17PM +0100, Jeremi Piotrowski wrote: >> Shouldn't this line be inside the cpu_has(c, X86_FEATURE_SEV_SNP) check? > > The cc_vendor is not dependent on X86_FEATURE_SEV_SNP. > It's not but if you set it before the check it will be set for all AMD systems, even if they are neither CC hosts nor CC guests. cc_vendor being unset is handled correctly in cc_platform_has() checks. >> How about turning this into a more specific check: >> >> if (!cc_platform_has(CC_ATTR_GUEST_SEV_SNP) && > > Why? > To leave open the possibility of an SNP hypervisor running nested. > The check is "am I running as a hypervisor on baremetal". > I thought you wanted to filter out SEV-SNP guests, which also have X86_FEATURE_SEV_SNP CPUID bit set. My understanding is that these are the cases: CPUID(SEV_SNP) | MSR(SEV_SNP) | what am I --------------------------------------------- set | set | SNP-guest set | unset | SNP-host unset | ?? | not SNP
On Thu, Mar 28, 2024 at 03:24:29PM +0100, Jeremi Piotrowski wrote: > It's not but if you set it before the check it will be set for all AMD > systems, even if they are neither CC hosts nor CC guests. That a problem? It is under a CONFIG_ARCH_HAS_CC_PLATFORM... > To leave open the possibility of an SNP hypervisor running nested. But !CC_ATTR_GUEST_SEV_SNP doesn't mean that. It means it is not a SEV-SNP guest. > I thought you wanted to filter out SEV-SNP guests, which also have > X86_FEATURE_SEV_SNP CPUID bit set. I want to run snp_probe_rmptable_info() only on baremetal where it makes sense. > My understanding is that these are the cases: > > CPUID(SEV_SNP) | MSR(SEV_SNP) | what am I > --------------------------------------------- > set | set | SNP-guest > set | unset | SNP-host > unset | ?? | not SNP So as you can see, we can't use X86_FEATURE_SEV_SNP for anything due to the late disable need. So we should be moving away from it. So we need a test for "am I a nested SNP hypervisor?" So, can your thing clear X86_FEATURE_HYPERVISOR and thus "emulate" baremetal?
On 3/27/24 10:43, Borislav Petkov wrote: > From: "Borislav Petkov (AMD)" <bp@alien8.de> > > The host SNP worthiness can determined later, after alternatives have > been patched, in snp_rmptable_init() depending on cmdline options like > iommu=pt which is incompatible with SNP, for example. > > Which means that one cannot use X86_FEATURE_SEV_SNP and will need to > have a special flag for that control. > > Use that newly added CC_ATTR_HOST_SEV_SNP in the appropriate places. > > Move kdump_sev_callback() to its rightfull place, while at it. > > Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> If late disabling of CPU feature flags is ever supported in the future, we should come back and possibly remove this. But until then... Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com> > --- > arch/x86/include/asm/sev.h | 4 ++-- > arch/x86/kernel/cpu/amd.c | 38 ++++++++++++++++++------------ > arch/x86/kernel/cpu/mtrr/generic.c | 2 +- > arch/x86/kernel/sev.c | 10 -------- > arch/x86/kvm/svm/sev.c | 2 +- > arch/x86/virt/svm/sev.c | 26 +++++++++++++------- > drivers/crypto/ccp/sev-dev.c | 2 +- > drivers/iommu/amd/init.c | 4 +++- > 8 files changed, 49 insertions(+), 39 deletions(-) >
On 28/03/2024 16:39, Borislav Petkov wrote: > On Thu, Mar 28, 2024 at 03:24:29PM +0100, Jeremi Piotrowski wrote: >> It's not but if you set it before the check it will be set for all AMD >> systems, even if they are neither CC hosts nor CC guests. > > That a problem? > No problem now but I did find it odd that cc_vendor will now always be set for AMD but not for Intel. For Intel the various checks would automatically return true. Something to look out for in the future when adding CC_ATTR's - no one can assume that the checks will only run when actively dealing with confidential computing. > It is under a CONFIG_ARCH_HAS_CC_PLATFORM... >>> To leave open the possibility of an SNP hypervisor running nested. > > But !CC_ATTR_GUEST_SEV_SNP doesn't mean that. It means it is not > a SEV-SNP guest. > >> I thought you wanted to filter out SEV-SNP guests, which also have >> X86_FEATURE_SEV_SNP CPUID bit set. > > I want to run snp_probe_rmptable_info() only on baremetal where it makes > sense. >>> My understanding is that these are the cases: >> >> CPUID(SEV_SNP) | MSR(SEV_SNP) | what am I >> --------------------------------------------- >> set | set | SNP-guest >> set | unset | SNP-host >> unset | ?? | not SNP > > So as you can see, we can't use X86_FEATURE_SEV_SNP for anything due to > the late disable need. So we should be moving away from it. > I see your point about the disable needing to happen late - but then how about we remove the setup_clear_cpu_cap(X86_FEATURE_SEV_SNP) too? No code depends on it any more and it would help my cause as well. > So we need a test for "am I a nested SNP hypervisor?" > > So, can your thing clear X86_FEATURE_HYPERVISOR and thus "emulate" > baremetal? > Can't do that... it is a VM and hypervisor detection and various paravirt interfaces depend on X86_FEATURE_HYPERVISOR.
On Thu, Apr 04, 2024 at 07:07:26PM +0200, Jeremi Piotrowski wrote: > On 28/03/2024 16:39, Borislav Petkov wrote: > > On Thu, Mar 28, 2024 at 03:24:29PM +0100, Jeremi Piotrowski wrote: > >> It's not but if you set it before the check it will be set for all AMD > >> systems, even if they are neither CC hosts nor CC guests. > > > > That a problem? > > > > No problem now but I did find it odd that cc_vendor will now always be set for AMD but > not for Intel. For Intel the various checks would automatically return true. Something > to look out for in the future when adding CC_ATTR's - no one can assume that the checks > will only run when actively dealing with confidential computing. Right, I haven't made up my mind fully here yet... setting cc_vendor *only* when running as some sort of a confidential computing guest kinda makes sense. And if it is not set, then that can be used to catch cases where the cc_* helpers are used outside of confidential computing cases... Do we want those assertions? I don't know... > I see your point about the disable needing to happen late - but then how about we remove > the setup_clear_cpu_cap(X86_FEATURE_SEV_SNP) too? No code depends on it any more and it would > help my cause as well. > > > So we need a test for "am I a nested SNP hypervisor?" > > > > So, can your thing clear X86_FEATURE_HYPERVISOR and thus "emulate" > > baremetal? > > > > Can't do that... it is a VM and hypervisor detection and various paravirt interfaces depend on > X86_FEATURE_HYPERVISOR. Right, but "your cause" as you call it above looks like a constant whack'a'mole game everytime we change something in the kernel when enabling those things and that breaks your cause. Do you really want that? Or would you prefer to define your nested solution properly and then have upstream code support it like the next well-defined coco platform instead? Thx.
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 9477b4053bce..780182cda3ab 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -228,7 +228,6 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, struct sn void snp_accept_memory(phys_addr_t start, phys_addr_t end); u64 snp_get_unsupported_features(u64 status); u64 sev_get_status(void); -void kdump_sev_callback(void); void sev_show_status(void); #else static inline void sev_es_ist_enter(struct pt_regs *regs) { } @@ -258,7 +257,6 @@ static inline int snp_issue_guest_request(u64 exit_code, struct snp_req_data *in static inline void snp_accept_memory(phys_addr_t start, phys_addr_t end) { } static inline u64 snp_get_unsupported_features(u64 status) { return 0; } static inline u64 sev_get_status(void) { return 0; } -static inline void kdump_sev_callback(void) { } static inline void sev_show_status(void) { } #endif @@ -270,6 +268,7 @@ int psmash(u64 pfn); int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 asid, bool immutable); int rmp_make_shared(u64 pfn, enum pg_level level); void snp_leak_pages(u64 pfn, unsigned int npages); +void kdump_sev_callback(void); #else static inline bool snp_probe_rmptable_info(void) { return false; } static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; } @@ -282,6 +281,7 @@ static inline int rmp_make_private(u64 pfn, u64 gpa, enum pg_level level, u32 as } static inline int rmp_make_shared(u64 pfn, enum pg_level level) { return -ENODEV; } static inline void snp_leak_pages(u64 pfn, unsigned int npages) {} +static inline void kdump_sev_callback(void) { } #endif #endif diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c index 6d8677e80ddb..9bf17c9c29da 100644 --- a/arch/x86/kernel/cpu/amd.c +++ b/arch/x86/kernel/cpu/amd.c @@ -345,6 +345,28 @@ static void srat_detect_node(struct cpuinfo_x86 *c) #endif } +static void bsp_determine_snp(struct cpuinfo_x86 *c) +{ +#ifdef CONFIG_ARCH_HAS_CC_PLATFORM + cc_vendor = CC_VENDOR_AMD; + + if (cpu_has(c, X86_FEATURE_SEV_SNP)) { + /* + * RMP table entry format is not architectural and is defined by the + * per-processor PPR. Restrict SNP support on the known CPU models + * for which the RMP table entry format is currently defined for. + */ + if (!cpu_has(c, X86_FEATURE_HYPERVISOR) && + c->x86 >= 0x19 && snp_probe_rmptable_info()) { + cc_platform_set(CC_ATTR_HOST_SEV_SNP); + } else { + setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); + cc_platform_clear(CC_ATTR_HOST_SEV_SNP); + } + } +#endif +} + static void bsp_init_amd(struct cpuinfo_x86 *c) { if (cpu_has(c, X86_FEATURE_CONSTANT_TSC)) { @@ -452,21 +474,7 @@ static void bsp_init_amd(struct cpuinfo_x86 *c) break; } - if (cpu_has(c, X86_FEATURE_SEV_SNP)) { - /* - * RMP table entry format is not architectural and it can vary by processor - * and is defined by the per-processor PPR. Restrict SNP support on the - * known CPU model and family for which the RMP table entry format is - * currently defined for. - */ - if (!boot_cpu_has(X86_FEATURE_ZEN3) && - !boot_cpu_has(X86_FEATURE_ZEN4) && - !boot_cpu_has(X86_FEATURE_ZEN5)) - setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); - else if (!snp_probe_rmptable_info()) - setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); - } - + bsp_determine_snp(c); return; warn: diff --git a/arch/x86/kernel/cpu/mtrr/generic.c b/arch/x86/kernel/cpu/mtrr/generic.c index 422a4ddc2ab7..7b29ebda024f 100644 --- a/arch/x86/kernel/cpu/mtrr/generic.c +++ b/arch/x86/kernel/cpu/mtrr/generic.c @@ -108,7 +108,7 @@ static inline void k8_check_syscfg_dram_mod_en(void) (boot_cpu_data.x86 >= 0x0f))) return; - if (cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return; rdmsr(MSR_AMD64_SYSCFG, lo, hi); diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index b59b09c2f284..1e1a3c3bd1e8 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -2287,16 +2287,6 @@ static int __init snp_init_platform_device(void) } device_initcall(snp_init_platform_device); -void kdump_sev_callback(void) -{ - /* - * Do wbinvd() on remote CPUs when SNP is enabled in order to - * safely do SNP_SHUTDOWN on the local CPU. - */ - if (cpu_feature_enabled(X86_FEATURE_SEV_SNP)) - wbinvd(); -} - void sev_show_status(void) { int i; diff --git a/arch/x86/kvm/svm/sev.c b/arch/x86/kvm/svm/sev.c index ae0ac12382b9..3d310b473e05 100644 --- a/arch/x86/kvm/svm/sev.c +++ b/arch/x86/kvm/svm/sev.c @@ -3174,7 +3174,7 @@ struct page *snp_safe_alloc_page(struct kvm_vcpu *vcpu) unsigned long pfn; struct page *p; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return alloc_page(GFP_KERNEL_ACCOUNT | __GFP_ZERO); /* diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c index cffe1157a90a..ab0e8448bb6e 100644 --- a/arch/x86/virt/svm/sev.c +++ b/arch/x86/virt/svm/sev.c @@ -77,7 +77,7 @@ static int __mfd_enable(unsigned int cpu) { u64 val; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return 0; rdmsrl(MSR_AMD64_SYSCFG, val); @@ -98,7 +98,7 @@ static int __snp_enable(unsigned int cpu) { u64 val; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return 0; rdmsrl(MSR_AMD64_SYSCFG, val); @@ -174,11 +174,11 @@ static int __init snp_rmptable_init(void) u64 rmptable_size; u64 val; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return 0; if (!amd_iommu_snp_en) - return 0; + goto nosnp; if (!probed_rmp_size) goto nosnp; @@ -225,7 +225,7 @@ static int __init snp_rmptable_init(void) return 0; nosnp: - setup_clear_cpu_cap(X86_FEATURE_SEV_SNP); + cc_platform_clear(CC_ATTR_HOST_SEV_SNP); return -ENOSYS; } @@ -246,7 +246,7 @@ static struct rmpentry *__snp_lookup_rmpentry(u64 pfn, int *level) { struct rmpentry *large_entry, *entry; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return ERR_PTR(-ENODEV); entry = get_rmpentry(pfn); @@ -363,7 +363,7 @@ int psmash(u64 pfn) unsigned long paddr = pfn << PAGE_SHIFT; int ret; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return -ENODEV; if (!pfn_valid(pfn)) @@ -472,7 +472,7 @@ static int rmpupdate(u64 pfn, struct rmp_state *state) unsigned long paddr = pfn << PAGE_SHIFT; int ret, level; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return -ENODEV; level = RMP_TO_PG_LEVEL(state->pagesize); @@ -558,3 +558,13 @@ void snp_leak_pages(u64 pfn, unsigned int npages) spin_unlock(&snp_leaked_pages_list_lock); } EXPORT_SYMBOL_GPL(snp_leak_pages); + +void kdump_sev_callback(void) +{ + /* + * Do wbinvd() on remote CPUs when SNP is enabled in order to + * safely do SNP_SHUTDOWN on the local CPU. + */ + if (cc_platform_has(CC_ATTR_HOST_SEV_SNP)) + wbinvd(); +} diff --git a/drivers/crypto/ccp/sev-dev.c b/drivers/crypto/ccp/sev-dev.c index f44efbb89c34..2102377f727b 100644 --- a/drivers/crypto/ccp/sev-dev.c +++ b/drivers/crypto/ccp/sev-dev.c @@ -1090,7 +1090,7 @@ static int __sev_snp_init_locked(int *error) void *arg = &data; int cmd, rc = 0; - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return -ENODEV; sev = psp->sev_data; diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c index e7a44929f0da..33228c1c8980 100644 --- a/drivers/iommu/amd/init.c +++ b/drivers/iommu/amd/init.c @@ -3228,7 +3228,7 @@ static bool __init detect_ivrs(void) static void iommu_snp_enable(void) { #ifdef CONFIG_KVM_AMD_SEV - if (!cpu_feature_enabled(X86_FEATURE_SEV_SNP)) + if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) return; /* * The SNP support requires that IOMMU must be enabled, and is @@ -3236,12 +3236,14 @@ static void iommu_snp_enable(void) */ if (no_iommu || iommu_default_passthrough()) { pr_err("SNP: IOMMU disabled or configured in passthrough mode, SNP cannot be supported.\n"); + cc_platform_clear(CC_ATTR_HOST_SEV_SNP); return; } amd_iommu_snp_en = check_feature(FEATURE_SNP); if (!amd_iommu_snp_en) { pr_err("SNP: IOMMU SNP feature not enabled, SNP cannot be supported.\n"); + cc_platform_clear(CC_ATTR_HOST_SEV_SNP); return; }