| Message ID | 8f73fc5a68f6713ba7ae1cbdbb7418145c4bd190.1738274758.git.ashish.kalra@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Fix broken SNP support with KVM module built-in | expand |
On Fri, Jan 31, 2025, Ashish Kalra wrote: > From: Ashish Kalra <ashish.kalra@amd.com> > > This patch fixes issues with enabling SNP host support and effectively ^^^^^^^^^^ > --- > arch/x86/include/asm/sev.h | 2 ++ > arch/x86/virt/svm/sev.c | 23 +++++++---------------- > 2 files changed, 9 insertions(+), 16 deletions(-) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index 5d9685f92e5c..1581246491b5 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -531,6 +531,7 @@ static inline void __init snp_secure_tsc_init(void) { } > > #ifdef CONFIG_KVM_AMD_SEV > bool snp_probe_rmptable_info(void); > +int snp_rmptable_init(void); > int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level); > void snp_dump_hva_rmpentry(unsigned long address); > int psmash(u64 pfn); > @@ -541,6 +542,7 @@ void kdump_sev_callback(void); > void snp_fixup_e820_tables(void); > #else > static inline bool snp_probe_rmptable_info(void) { return false; } > +static inline int snp_rmptable_init(void) { return -ENOSYS; } > static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; } > static inline void snp_dump_hva_rmpentry(unsigned long address) {} > static inline int psmash(u64 pfn) { return -ENODEV; } > diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c > index 1dcc027ec77e..42e74a5a7d78 100644 > --- a/arch/x86/virt/svm/sev.c > +++ b/arch/x86/virt/svm/sev.c > @@ -505,19 +505,19 @@ static bool __init setup_rmptable(void) > * described in the SNP_INIT_EX firmware command description in the SNP > * firmware ABI spec. > */ > -static int __init snp_rmptable_init(void) > +int __init snp_rmptable_init(void) > { > unsigned int i; > u64 val; > > - if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) > - return 0; > + if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP))) > + return -ENOSYS; > > - if (!amd_iommu_snp_en) > - goto nosnp; > + if (WARN_ON_ONCE(!amd_iommu_snp_en)) > + return -ENOSYS; > > if (!setup_rmptable()) > - goto nosnp; > + return -ENOSYS; > > /* > * Check if SEV-SNP is already enabled, this can happen in case of > @@ -530,7 +530,7 @@ static int __init snp_rmptable_init(void) > /* Zero out the RMP bookkeeping area */ > if (!clear_rmptable_bookkeeping()) { > free_rmp_segment_table(); > - goto nosnp; > + return -ENOSYS; > } > > /* Zero out the RMP entries */ > @@ -562,17 +562,8 @@ static int __init snp_rmptable_init(void) > crash_kexec_post_notifiers = true; > > return 0; > - > -nosnp: > - cc_platform_clear(CC_ATTR_HOST_SEV_SNP); > - return -ENOSYS; > } > > -/* > - * This must be called after the IOMMU has been initialized. > - */ > -device_initcall(snp_rmptable_init); There's the wee little problem that snp_rmptable_init() is never called as of this patch. Dropping the device_initcall() needs to happen in the same patch that wires up the IOMMU code to invoke snp_rmptable_init(). At a glance, I don't see anything in this patch that can reasonably go in before the IOMMU change.
Hello Sean, On 1/30/2025 7:41 PM, Sean Christopherson wrote: > On Fri, Jan 31, 2025, Ashish Kalra wrote: >> From: Ashish Kalra <ashish.kalra@amd.com> >> >> This patch fixes issues with enabling SNP host support and effectively > ^^^^^^^^^^ > >> --- >> arch/x86/include/asm/sev.h | 2 ++ >> arch/x86/virt/svm/sev.c | 23 +++++++---------------- >> 2 files changed, 9 insertions(+), 16 deletions(-) >> >> diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h >> index 5d9685f92e5c..1581246491b5 100644 >> --- a/arch/x86/include/asm/sev.h >> +++ b/arch/x86/include/asm/sev.h >> @@ -531,6 +531,7 @@ static inline void __init snp_secure_tsc_init(void) { } >> >> #ifdef CONFIG_KVM_AMD_SEV >> bool snp_probe_rmptable_info(void); >> +int snp_rmptable_init(void); >> int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level); >> void snp_dump_hva_rmpentry(unsigned long address); >> int psmash(u64 pfn); >> @@ -541,6 +542,7 @@ void kdump_sev_callback(void); >> void snp_fixup_e820_tables(void); >> #else >> static inline bool snp_probe_rmptable_info(void) { return false; } >> +static inline int snp_rmptable_init(void) { return -ENOSYS; } >> static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; } >> static inline void snp_dump_hva_rmpentry(unsigned long address) {} >> static inline int psmash(u64 pfn) { return -ENODEV; } >> diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c >> index 1dcc027ec77e..42e74a5a7d78 100644 >> --- a/arch/x86/virt/svm/sev.c >> +++ b/arch/x86/virt/svm/sev.c >> @@ -505,19 +505,19 @@ static bool __init setup_rmptable(void) >> * described in the SNP_INIT_EX firmware command description in the SNP >> * firmware ABI spec. >> */ >> -static int __init snp_rmptable_init(void) >> +int __init snp_rmptable_init(void) >> { >> unsigned int i; >> u64 val; >> >> - if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) >> - return 0; >> + if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP))) >> + return -ENOSYS; >> >> - if (!amd_iommu_snp_en) >> - goto nosnp; >> + if (WARN_ON_ONCE(!amd_iommu_snp_en)) >> + return -ENOSYS; >> >> if (!setup_rmptable()) >> - goto nosnp; >> + return -ENOSYS; >> >> /* >> * Check if SEV-SNP is already enabled, this can happen in case of >> @@ -530,7 +530,7 @@ static int __init snp_rmptable_init(void) >> /* Zero out the RMP bookkeeping area */ >> if (!clear_rmptable_bookkeeping()) { >> free_rmp_segment_table(); >> - goto nosnp; >> + return -ENOSYS; >> } >> >> /* Zero out the RMP entries */ >> @@ -562,17 +562,8 @@ static int __init snp_rmptable_init(void) >> crash_kexec_post_notifiers = true; >> >> return 0; >> - >> -nosnp: >> - cc_platform_clear(CC_ATTR_HOST_SEV_SNP); >> - return -ENOSYS; >> } >> >> -/* >> - * This must be called after the IOMMU has been initialized. >> - */ >> -device_initcall(snp_rmptable_init); > > There's the wee little problem that snp_rmptable_init() is never called as of > this patch. Dropping the device_initcall() needs to happen in the same patch > that wires up the IOMMU code to invoke snp_rmptable_init(). The issue with that is the IOMMU and x86 maintainers are different, so i believe that we will need to split the dropping of device_initcall() in platform code and the code to wire up the IOMMU driver to invoke snp_rmptable_init(), to get the patch merged in different trees ? >At a glance, I don't see anything in this patch that can reasonably go in before the IOMMU change. This patch prepares snp_rmptable_init() to be called via iommu_snp_enable(), so i assume this is a pre-patch before the IOMMU change. Thanks, Ashish
diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 5d9685f92e5c..1581246491b5 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -531,6 +531,7 @@ static inline void __init snp_secure_tsc_init(void) { } #ifdef CONFIG_KVM_AMD_SEV bool snp_probe_rmptable_info(void); +int snp_rmptable_init(void); int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level); void snp_dump_hva_rmpentry(unsigned long address); int psmash(u64 pfn); @@ -541,6 +542,7 @@ void kdump_sev_callback(void); void snp_fixup_e820_tables(void); #else static inline bool snp_probe_rmptable_info(void) { return false; } +static inline int snp_rmptable_init(void) { return -ENOSYS; } static inline int snp_lookup_rmpentry(u64 pfn, bool *assigned, int *level) { return -ENODEV; } static inline void snp_dump_hva_rmpentry(unsigned long address) {} static inline int psmash(u64 pfn) { return -ENODEV; } diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c index 1dcc027ec77e..42e74a5a7d78 100644 --- a/arch/x86/virt/svm/sev.c +++ b/arch/x86/virt/svm/sev.c @@ -505,19 +505,19 @@ static bool __init setup_rmptable(void) * described in the SNP_INIT_EX firmware command description in the SNP * firmware ABI spec. */ -static int __init snp_rmptable_init(void) +int __init snp_rmptable_init(void) { unsigned int i; u64 val; - if (!cc_platform_has(CC_ATTR_HOST_SEV_SNP)) - return 0; + if (WARN_ON_ONCE(!cc_platform_has(CC_ATTR_HOST_SEV_SNP))) + return -ENOSYS; - if (!amd_iommu_snp_en) - goto nosnp; + if (WARN_ON_ONCE(!amd_iommu_snp_en)) + return -ENOSYS; if (!setup_rmptable()) - goto nosnp; + return -ENOSYS; /* * Check if SEV-SNP is already enabled, this can happen in case of @@ -530,7 +530,7 @@ static int __init snp_rmptable_init(void) /* Zero out the RMP bookkeeping area */ if (!clear_rmptable_bookkeeping()) { free_rmp_segment_table(); - goto nosnp; + return -ENOSYS; } /* Zero out the RMP entries */ @@ -562,17 +562,8 @@ static int __init snp_rmptable_init(void) crash_kexec_post_notifiers = true; return 0; - -nosnp: - cc_platform_clear(CC_ATTR_HOST_SEV_SNP); - return -ENOSYS; } -/* - * This must be called after the IOMMU has been initialized. - */ -device_initcall(snp_rmptable_init); - static void set_rmp_segment_info(unsigned int segment_shift) { rmp_segment_shift = segment_shift;