Re: [PATCH v2 4/4] iommu/amd: Enable Host SNP support after enabling IOMMU SNP support

[Sean Christopherson]

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)