Hello All,
On 1/22/2025 11:07 AM, Vasant Hegde wrote:
> Hi Tom,
>
>
> On 1/22/2025 8:52 PM, Tom Lendacky wrote:
>> On 1/21/25 19:00, Ashish Kalra wrote:
>>> From: Vasant Hegde <vasant.hegde@xxxxxxx>
>>>
>>> iommu_snp_enable() checks for IOMMU feature support and page table
>>> compatibility. Ideally this check should be done before enabling
>>> IOMMUs. Currently its done after enabling IOMMUs. Also its causes
>>
>> Why should it be done before enabling the IOMMUs? In other words, at
>> some more detail here.
>
> Sure. Basically IOMMU enable stage checks for SNP support. I will update it.
>
>>
>>> issue if kvm_amd is builtin.
>>>
>>> Hence move SNP enable check before enabling IOMMUs.
>>>
>>> Fixes: 04d65a9dbb33 ("iommu/amd: Don't rely on external callers to enable IOMMU SNP support")
>>> Cc: Ashish Kalra <ashish.kalra@xxxxxxx>
>>> Signed-off-by: Vasant Hegde <vasant.hegde@xxxxxxx>
>>
>> Ashish, as the submitter, this requires your Signed-off-by:.
>>
>>> ---
>>> drivers/iommu/amd/init.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
>>> index c5cd92edada0..419a0bc8eeea 100644
>>> --- a/drivers/iommu/amd/init.c
>>> +++ b/drivers/iommu/amd/init.c
>>> @@ -3256,13 +3256,14 @@ static int __init state_next(void)
>>> }
>>> break;
>>> case IOMMU_ACPI_FINISHED:
>>> + /* SNP enable has to be called after early_amd_iommu_init() */
>>
>> This comment doesn't really explain anything, so I think it should
>> either be improved or just remove it.
>
> Sure. I will remove it.
We have hit a blocker issue with this patch.
With discussions with the AMD IOMMU team, here is the AMD IOMMU initialization flow:
IOMMU initialization happens in various stages.
1) Detect IOMMU Presence
start_kernel() -> mm_core_init() -> mem_init() -> pci_iommu_alloc() -> amd_iommu_detect()
At this stage memory subsystem is not initialized completely. So we just detect the IOMMU presence.
2) Interrupt Remapping
During APIC init it checks for IOMMU interrupt remapping. At this stage, we initialize the IOMMU and
enable the IOMMU.
start_kernel() -> x86_late_time_init() -> apic_intr_mode_init() -> x86_64_probe_apic() ->
enable_IR_x2apic() -> irq_remapping_prepare() -> amd_iommu_prepare()
3) PCI init
This is done using rootfs_initcall(pci_iommu_init);
pci_iommu_init() -> amd_iommu_init()
At this stage we enable the IOMMU interrupt, probe device etc. IOMMU is ready to use.
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.
As discussed internally, we have 2 possible options to fix this:
1 ) Similar to calling sp_mod_init() to explicitly initialize the PSP driver, we call snp_rmptable_init() from KVM_AMD if it's built-in.
So that we don't need changes to IOMMU driver ... as IOMMU driver does SNP check as part of rootfs_initcall() (amd_iommu_init())
2) Rework it such that
Core IOMMU (iommu_subsys_init()) initialized (as currently) via subsys_initcall
FIX:
And then we add a fix to invoke snp_rmptable_init() via a subsys_initcall_sync() (instead of device_initcall()).
(again as core IOMMU subsystem init is called by subsys_initcall())
--> snp_rmptable_init() will additionally need to call iommu_snp_enable() to check and enable SNP support on the IOMMU.
Issues with option (1):
Here, snp_rmptable_init() is still invoked via device_initcall() for normal module loading case and i remember Sean had concerns of enabling
host SNP at device_initcall level generally as it is too late, though looking at AMD IOMMU driver initialization flow, i don't think there is
much of a choice here.
One other possibility is moving snp_rmptable_init() call to KVM initialization, but that has issues with PSP driver loading and
initializing before KVM (with module loading case) and that will cause PSP's SEV/SNP init to fail as SNP is not enabled yet.
But again that will work when SEV/SNP init will move to KVM module load time where PSP module will be simply be loaded and initialized
before KVM, but will not attempt to do SEV/SNP init.
This approach is quite fragile and will need to be tested and needs to work with multiple scenarios and cases as explained above, there is
a good chance of this breaking something.
And that's why option (2) is preferred.
Issues with option (2):
How to call iommu_snp_enable() from snp_rmptable_init() ?
We probably can't call iommu_snp_enable() explicitly from snp_rmptable_init() as i remember the last time we proposed this, it was rejected as
maintainers were not in favor of core kernel code calling driver functions, though the AMD IOMMU driver is always built-in ?
Therefore, probably the approach will be something like AMD_IOMMU driver registering some kind of callback interface and that callback is
invoked via snp_rmptable_init() to check and enable SNP support on the IOMMU.
Boris, it will be nice to have your feedback/thoughts on this approach/option?
Looking fwd. to more feedback/thoughts/comments on the above.
Thanks,
Ashish
