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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <a4ed411d-3233-4d15-94a0-8da177633073@amd.com>
Date: Wed, 29 Jan 2025 14:54:31 +0530
From: Vasant Hegde <vasant.hegde@....com>
To: Sean Christopherson <seanjc@...gle.com>,
 Ashish Kalra <ashish.kalra@....com>
Cc: Tom Lendacky <thomas.lendacky@....com>, pbonzini@...hat.com,
 tglx@...utronix.de, mingo@...hat.com, bp@...en8.de,
 dave.hansen@...ux.intel.com, x86@...nel.org, hpa@...or.com,
 john.allen@....com, herbert@...dor.apana.org.au, davem@...emloft.net,
 joro@...tes.org, suravee.suthikulpanit@....com, will@...nel.org,
 robin.murphy@....com, michael.roth@....com, dionnaglaze@...gle.com,
 kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
 linux-crypto@...r.kernel.org, linux-coco@...ts.linux.dev,
 iommu@...ts.linux.dev
Subject: Re: [PATCH 1/4] iommu/amd: Check SNP support before enabling IOMMU

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@....com>
Date:   Wed Jan 29 14:47:04 2025 +0530

    iommu/amd: SNP fix

    Signed-off-by: Vasant Hegde <vasant.hegde@....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;
>  }