Re: [PATCH 1/4] iommu/amd: Check SNP support before enabling IOMMU

[Vasant Hegde <vasant.hegde@amd.com>]

Hi Sean,


On 1/28/2025 2:42 AM, Sean Christopherson wrote:
> On Mon, Jan 27, 2025, Ashish Kalra wrote:
>> Hello Sean,
>>
>> On 1/24/2025 6:39 PM, Sean Christopherson wrote:
>>> On Fri, Jan 24, 2025, Ashish Kalra wrote:
>>>> With discussions with the AMD IOMMU team, here is the AMD IOMMU
>>>> initialization flow:
>>>
>>> ..
>>>
>>>> IOMMU SNP check
>>>>   Core IOMMU subsystem init is done during iommu_subsys_init() via
>>>>   subsys_initcall.  This function does change the DMA mode depending on
>>>>   kernel config.  Hence, SNP check should be done after subsys_initcall.
>>>>   That's why its done currently during IOMMU PCI init (IOMMU_PCI_INIT stage).
>>>>   And for that reason snp_rmptable_init() is currently invoked via
>>>>   device_initcall().
>>>>  
>>>> The summary is that we cannot move snp_rmptable_init() to subsys_initcall as
>>>> core IOMMU subsystem gets initialized via subsys_initcall.
>>>
>>> Just explicitly invoke RMP initialization during IOMMU SNP setup.  Pretending
>>> there's no connection when snp_rmptable_init() checks amd_iommu_snp_en and has
>>> a comment saying it needs to come after IOMMU SNP setup is ridiculous.
>>>
>>
>> Thanks for the suggestion and the patch, i have tested it works for all cases
>> and scenarios. I will post the next version of the patch-set based on this
>> patch.
> 
> One thing I didn't account for: if IOMMU initialization fails and iommu_snp_enable()
> is never reached, CC_ATTR_HOST_SEV_SNP will be left set.
> 
> I don't see any great options.  Something like the below might work?  And maybe

We did explore few other options. But I don't see any other better option.

Below code works fine.  But we still need to handle `iommu=off` or
`amd_iommu=off` kernel command line. Below change will take care of this
scenario. Does this looks OK?

----
commit 8e9296346e8f6a0831a5f6076c81a636bf044a41
Author: Vasant Hegde <vasant.hegde@amd.com>
Date:   Wed Jan 29 14:47:04 2025 +0530

    iommu/amd: SNP fix

    Signed-off-by: Vasant Hegde <vasant.hegde@amd.com>

diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
index c5cd92edada0..08802316411f 100644
--- a/drivers/iommu/amd/init.c
+++ b/drivers/iommu/amd/init.c
@@ -3426,18 +3426,24 @@ 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;

 	amd_iommu_detected = true;
 	iommu_detected = 1;
 	x86_init.iommu.iommu_init = amd_iommu_init;
+	return;
+
+disable_snp:
+	/* Disable SNP if amd_iommu is not enabled */
+	if (cc_platform_has(CC_ATTR_HOST_SEV_SNP))
+		cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
 }

 /****************************************************************************




> keep a device_initcall() in arch/x86/virt/svm/sev.c that sanity checks that SNP
> really is fully enabled?  Dunno, hopefully someone has a better idea.

That will not solve the initial problem this series trying to solve (i. e.
kvm_amd as built and making sure SNP init happens before device_initcall() path).

I think with your patch and above changes it should work fine.

-Vasant


> 
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 0e0a531042ac..6d62ee8e0055 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3295,6 +3295,9 @@ static int __init iommu_go_to_state(enum iommu_init_state state)
>                 ret = state_next();
>         }
>  
> +       if (ret && !amd_iommu_snp_en)
> +               cc_platform_clear(CC_ATTR_HOST_SEV_SNP);
> +
>         return ret;
>  }