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@xxxxxxx>
Date: Wed Jan 29 14:47:04 2025 +0530
iommu/amd: SNP fix
Signed-off-by: Vasant Hegde <vasant.hegde@xxxxxxx>
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;
> }
