Re: [PATCH v10 06/50] x86/sev: Add the host SEV-SNP initialization support

[Kalra, Ashish]

On 11/7/2023 2:27 PM, Borislav Petkov wrote:
> On Tue, Nov 07, 2023 at 08:19:31PM +0100, Borislav Petkov wrote:
> > Arch code does not call drivers - arch code sets up the arch and
> > provides facilities which the drivers use.
>
> IOW (just an example diff):
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 1c9924de607a..00cdbc844961 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3290,6 +3290,7 @@ static int __init state_next(void)
>   		break;
>   	case IOMMU_ENABLED:
>   		register_syscore_ops(&amd_iommu_syscore_ops);
> +		amd_iommu_snp_enable();
>   		ret = amd_iommu_init_pci();
>   		init_state = ret ? IOMMU_INIT_ERROR : IOMMU_PCI_INIT;
>   		break;
> @@ -3814,16 +3815,6 @@ int amd_iommu_snp_enable(void)
>   		return -EINVAL;
>   	}
>   
> -	/*
> -	 * Prevent enabling SNP after IOMMU_ENABLED state because this process
> -	 * affect how IOMMU driver sets up data structures and configures
> -	 * IOMMU hardware.
> -	 */
> -	if (init_state > IOMMU_ENABLED) {
> -		pr_err("SNP: Too late to enable SNP for IOMMU.\n");
> -		return -EINVAL;
> -	}
> -
>   	amd_iommu_snp_en = check_feature_on_all_iommus(FEATURE_SNP);
>   	if (!amd_iommu_snp_en)
>   		return -EINVAL;
>
> and now you only need to line up snp_rmptable_init() after IOMMU init
> instead of having it be a fs_initcall which happens right after
> pci_subsys_init() so that PCI is there but at the right time when iommu
> init state is at IOMMU_ENABLED but no later because then it is too late.
>
> And there you need to test amd_iommu_snp_en which is already exported
> anyway.
>
> Ok?

No, this is not correct as this will always enable SNP support on IOMMU 
even when SNP support is not supported and enabled on the platform, and 
then we will do stuff like forcing IOMMU v1 pagetables which we really 
don't want to do if SNP is not supported and enabled on the platform.

That's what snp_rmptable_init() calling amd_iommu_snp_enable() ensures 
that SNP on IOMMU is *only* enabled when platform/arch support for it is 
detected and enabled.

And isn't IOMMU driver always going to be built-in and isn't it part of 
the platform support (not arch code, but surely platform specific code)?
(IOMMU enablement is requirement for SNP).

Thanks,
Ashish