Hello Sean, On 2/7/2025 9:52 AM, Sean Christopherson wrote: > On Wed, Feb 05, 2025, Ashish Kalra wrote: >> On 2/5/2025 1:31 PM, Sean Christopherson wrote: >>> On Wed, Feb 05, 2025, Vasant Hegde wrote: >>>> So we don't want to clear CC_ATTR_HOST_SEV_SNP after RMP initialization -OR- >>>> clear for all failures? >>> >>> I honestly don't know, because the answer largely depends on what happens with >>> hardware. I asked in an earlier version of this series if IOMMU initialization >>> failure after the RMP is configured is even survivable. >>> >> >> As i mentioned earlier and as part of this series and summarizing this again here: > > Thanks! > >> - snp_rmptable_init() enables SNP support system-wide and that means the HW starts >> doing RMP checks for memory accesses, but as RMP table is zeroed out initially, >> all memory is configured to be host/HV owned. >> >> It is only after SNP_INIT(_EX) that RMP table is configured and initialized with >> HV_Fixed, firmware pages and stuff like IOMMU RMP enforcement is enabled. >> >> If the IOMMU initialization fails after IOMMU support on SNP check is completed >> and host SNP is enabled, then SNP_INIT(_EX) will fail as IOMMUs need to be enabled >> for SNP_INIT to succeed. >> >>> For this series, I think it makes sense to match the existing behavior, unless >>> someone from AMD can definitively state that we should do something different. >>> And the existing behavior is that amd_iommu_snp_en and CC_ATTR_HOST_SEV_SNP will >>> be left set if the IOMMU completes iommu_snp_enable(), and the kernel completes >>> RMP setup. >> >> Yes, that is true and this behavior is still consistent with this series. >> >> Again to reiterate, if iommu_snp_enable() and host SNP enablement is successful, >> any late IOMMU initialization failures should cause SNP_INIT to fail and that means >> IOMMU RMP enforcement will never get enabled and RMP table will remain configured >> for all memory marked as HV/host owned. > > So the kernel should be able to limp along, but CC_ATTR_HOST_SEV_SNP will be in > a half-baked state. > > Would it make sense to WARN if the RMP has been configured? E.g. as a follow-up > change: > > /* > * SNP platform initilazation requires IOMMUs to be fully configured. > * If the RMP has NOT been configured, simply mark SNP as unsupported. > * If the RMP is configured, but RMP enforcement has not been enabled > * in IOMMUs, then the system is in a half-baked state, but can limp > * along as all memory should be Hypervisor-Owned in the RMP. WARN, > * but leave SNP as "supported" to avoid confusing the kernel. > */ > if (ret && cc_platform_has(CC_ATTR_HOST_SEV_SNP) && > !WARN_ON_ONCE(amd_iommu_snp_en)) > cc_platform_clear(CC_ATTR_HOST_SEV_SNP); Yes, i can re-spin the series with this WARN_ON() added and additional comments added. Thanks, Ashish
