On Wed, Feb 05, 2025, Vasant Hegde wrote: > On 1/31/2025 7:18 AM, Sean Christopherson wrote: > > On Fri, Jan 31, 2025, Ashish Kalra wrote: > >> @@ -3426,18 +3431,23 @@ void __init amd_iommu_detect(void) > >> int ret; > >> > >> if (no_iommu || (iommu_detected && !gart_iommu_aperture)) > >> - return; > >> + goto disable_snp; > >> > >> if (!amd_iommu_sme_check()) > >> - return; > >> + goto disable_snp; > >> > >> ret = iommu_go_to_state(IOMMU_IVRS_DETECTED); > >> if (ret) > >> - return; > >> + goto disable_snp; > > > > This handles initial failure, but it won't handle the case where amd_iommu_prepare() > > fails, as the iommu_go_to_state() call from amd_iommu_enable() will get > > short-circuited. I don't see any pleasant options. Maybe this? > > > > diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c > > index c5cd92edada0..436e47f13f8f 100644 > > --- a/drivers/iommu/amd/init.c > > +++ b/drivers/iommu/amd/init.c > > @@ -3318,6 +3318,8 @@ static int __init iommu_go_to_state(enum iommu_init_state state) > > ret = state_next(); > > } > > > > + if (ret && !amd_iommu_snp_en && cc_platform_has(CC_ATTR_HOST_SEV_SNP)) > > I think we should clear when `amd_iommu_snp_en` is true. That doesn't address the case where amd_iommu_prepare() fails, because amd_iommu_snp_en will be %false (it's init value) and the RMP will be uninitialized, i.e. CC_ATTR_HOST_SEV_SNP will be incorrectly left set. And conversely, IMO clearing CC_ATTR_HOST_SEV_SNP after initializing the IOMMU and RMP is wrong as well. Such a host is probably hosted regardless, but from the CPU's perspective, SNP is supported and enabled. > May be below check is enough? > > if (ret && amd_iommu_snp_en)
