On 08/11/2023 07:14, Borislav Petkov wrote: > On Tue, Nov 07, 2023 at 04:33:41PM -0600, Kalra, Ashish wrote: >> We will still need some method to tell the IOMMU driver if SNP >> support/feature is disabled by this function, for example, when CPU family >> and model is not supported by SNP and we jump to no_snp label. > > See below. > >> The reliable way for this to work is to ensure snp_rmptable_init() is called >> before IOMMU initialization and then IOMMU initialization depends on SNP >> feature flag setup by snp_rmptable_init() to enable SNP support on IOMMU or >> not. > > Yes, this whole SNP initialization needs to be reworked and split this > way: I agree with Borislav and have some comments of my own. > > - early detection work which needs to be done once goes to > bsp_init_amd(): that's basically your early_detect_mem_encrypt() stuff > which needs to happen exactly only once and early. > > - Any work like: > > c->x86_phys_bits -= (cpuid_ebx(0x8000001f) >> 6) & 0x3f; > > and the like which needs to happen on each AP, gets put in a function > which gets called by init_amd(). > > By the time IOMMU gets to init, you already know whether it should > enable SNP and check X86_FEATURE_SEV_SNP. This flow would suit me better too. In SNP-host capable Hyper-V VMs there is no IOMMU and I've had to resort to early return from amd_iommu_snp_enable() to prevent it from disabling SNP [1]. In addition to what Borislav posted, you'd just need to enforce that if IOMMU is detected it actually gets enabled. [1]: https://lore.kernel.org/lkml/20230213103402.1189285-6-jpiotrowski@xxxxxxxxxxxxxxxxxxx/ > > Finally, you call __snp_rmptable_init() which does the *per-CPU* init > work which is still pending. Yes please, and the only rmp thing left to do per-CPU would be to check that the MSRs are set the same as the value read from CPU0. Running the early_rmptable_check() from early_detect_mem_encrypt() and on every CPU makes it difficult to support a kernel allocated RMP table. If you look at what I did for the mentioned Hyper-V SNP-host VMs [2] (which I think is reasonable) the RMP table is allocated in init_mem_mapping() and the addresses are propagated to other CPUs through hv_cpu_init(), which is called from cpuhp_setup_state(CPUHP_AP_HYPERV_ONLINE, ...). So it would be great if any init works plays nice with cpu hotplug notifiers. [2]: https://lore.kernel.org/lkml/20230213103402.1189285-2-jpiotrowski@xxxxxxxxxxxxxxxxxxx/ Thanks, Jeremi > > Ok? > > Ontop of the previous ontop patch: > > --- > diff --git a/arch/x86/kernel/cpu/amd.c b/arch/x86/kernel/cpu/amd.c > index 6cc2074fcea3..a9c95e5d6b06 100644 > --- a/arch/x86/kernel/cpu/amd.c > +++ b/arch/x86/kernel/cpu/amd.c > @@ -674,8 +674,19 @@ static void early_detect_mem_encrypt(struct cpuinfo_x86 *c) > if (!(msr & MSR_K7_HWCR_SMMLOCK)) > goto clear_sev; > > - if (cpu_has(c, X86_FEATURE_SEV_SNP) && !early_rmptable_check()) > - goto clear_snp; > + 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 (c->x86 != 0x19 || c->x86_model > 0xaf) > + goto clear_snp; > + > + if (!early_rmptable_check()) > + goto clear_snp; > + } > > return; > > diff --git a/arch/x86/virt/svm/sev.c b/arch/x86/virt/svm/sev.c > index 9237c327ad6d..5a71df9ae4cb 100644 > --- a/arch/x86/virt/svm/sev.c > +++ b/arch/x86/virt/svm/sev.c > @@ -199,14 +199,6 @@ static int __init snp_rmptable_init(void) > if (!amd_iommu_snp_en) > return 0; > > - /* > - * 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_data.x86 != 0x19 || boot_cpu_data.x86_model > 0xaf) > - goto nosnp; > - > if (__snp_rmptable_init()) > goto nosnp; >
