[Part2,v6,02/49] iommu/amd: Introduce function to check SEV-SNP support

From: Brijesh Singh <brijesh.singh@amd.com>

The SEV-SNP support requires that IOMMU must to enabled, see the IOMMU
spec section 2.12 for further details. If IOMMU is not enabled or the
SNPSup extended feature register is not set then the SNP_INIT command
(used for initializing firmware) will fail.

The iommu_sev_snp_supported() can be used to check if IOMMU supports the
SEV-SNP feature.

Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 drivers/iommu/amd/init.c | 30 ++++++++++++++++++++++++++++++
 include/linux/iommu.h    |  9 +++++++++
 2 files changed, 39 insertions(+)

On Mon, Jun 20, 2022 at 4:59 PM Ashish Kalra <Ashish.Kalra@amd.com> wrote:
>
> From: Brijesh Singh <brijesh.singh@amd.com>
>
> The SEV-SNP support requires that IOMMU must to enabled, see the IOMMU
> spec section 2.12 for further details. If IOMMU is not enabled or the
> SNPSup extended feature register is not set then the SNP_INIT command
> (used for initializing firmware) will fail.
>
> The iommu_sev_snp_supported() can be used to check if IOMMU supports the
> SEV-SNP feature.
>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  drivers/iommu/amd/init.c | 30 ++++++++++++++++++++++++++++++
>  include/linux/iommu.h    |  9 +++++++++
>  2 files changed, 39 insertions(+)
>
> diff --git a/drivers/iommu/amd/init.c b/drivers/iommu/amd/init.c
> index 1a3ad58ba846..82be8067ddf5 100644
> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -3361,3 +3361,33 @@ int amd_iommu_pc_set_reg(struct amd_iommu *iommu, u8 bank, u8 cntr, u8 fxn, u64
>
>         return iommu_pc_get_set_reg(iommu, bank, cntr, fxn, value, true);
>  }
> +
> +bool iommu_sev_snp_supported(void)
> +{
> +       struct amd_iommu *iommu;
> +
> +       /*
> +        * The SEV-SNP support requires that IOMMU must be enabled, and is
> +        * not configured in the passthrough mode.
> +        */
> +       if (no_iommu || iommu_default_passthrough()) {
> +               pr_err("SEV-SNP: IOMMU is either disabled or configured in passthrough mode.\n");

Like below could this say something like snp support is disabled
because of iommu settings.

> +               return false;
> +       }
> +
> +       /*
> +        * Iterate through all the IOMMUs and verify the SNPSup feature is
> +        * enabled.
> +        */
> +       for_each_iommu(iommu) {
> +               if (!iommu_feature(iommu, FEATURE_SNP)) {
> +                       pr_err("SNPSup is disabled (devid: %02x:%02x.%x)\n",

SNPSup might not be obvious to readers, what about " SNP Support is
disabled ...".

Also should this have the "SEV-SNP:" prefix like the above log?

> +                              PCI_BUS_NUM(iommu->devid), PCI_SLOT(iommu->devid),
> +                              PCI_FUNC(iommu->devid));
> +                       return false;
> +               }
> +       }
> +
> +       return true;
> +}
> +EXPORT_SYMBOL_GPL(iommu_sev_snp_supported);
> diff --git a/include/linux/iommu.h b/include/linux/iommu.h
> index 9208eca4b0d1..fecb72e1b11b 100644
> --- a/include/linux/iommu.h
> +++ b/include/linux/iommu.h
> @@ -675,6 +675,12 @@ struct iommu_sva *iommu_sva_bind_device(struct device *dev,
>  void iommu_sva_unbind_device(struct iommu_sva *handle);
>  u32 iommu_sva_get_pasid(struct iommu_sva *handle);
>
> +#ifdef CONFIG_AMD_MEM_ENCRYPT
> +bool iommu_sev_snp_supported(void);
> +#else
> +static inline bool iommu_sev_snp_supported(void) { return false; }
> +#endif
> +
>  #else /* CONFIG_IOMMU_API */
>
>  struct iommu_ops {};
> @@ -1031,6 +1037,9 @@ static inline struct iommu_fwspec *dev_iommu_fwspec_get(struct device *dev)
>  {
>         return NULL;
>  }
> +
> +static inline bool iommu_sev_snp_supported(void) { return false; }
> +
>  #endif /* CONFIG_IOMMU_API */
>
>  /**
> --
> 2.25.1
>

[AMD Official Use Only - General]

Hello Peter,

>> +bool iommu_sev_snp_supported(void)
>> +{
>> +       struct amd_iommu *iommu;
>> +
>> +       /*
>> +        * The SEV-SNP support requires that IOMMU must be enabled, and is
>> +        * not configured in the passthrough mode.
>> +        */
>> +       if (no_iommu || iommu_default_passthrough()) {
>> +               pr_err("SEV-SNP: IOMMU is either disabled or 
>> + configured in passthrough mode.\n");

> Like below could this say something like snp support is disabled because of iommu settings.

Here we may need to be more precise with the error information indicating why SNP is not enabled.
Please note that this patch may actually become part of the IOMMU + SNP patch series, where
additional checks are done, for example, not enabling SNP if IOMMU v2 page tables are enabled,
so precise error information will be useful here.

>> +               return false;
>> +       }
>> +
>> +       /*
>> +        * Iterate through all the IOMMUs and verify the SNPSup feature is
>> +        * enabled.
>> +        */
>> +       for_each_iommu(iommu) {
>> +               if (!iommu_feature(iommu, FEATURE_SNP)) {
>> +                       pr_err("SNPSup is disabled (devid: 
>> + %02x:%02x.%x)\n",

> SNPSup might not be obvious to readers, what about " SNP Support is disabled ...".

Yes, that makes sense.

> Also should this have the "SEV-SNP:" prefix like the above log?

Probably, we want to be more consistent with the SNP guest patches and replace
SEV-SNP with SNP everywhere, including function names, etc.

Thanks,
Ashish

On Tue, Jun 21, 2022 at 11:45 AM Kalra, Ashish <Ashish.Kalra@amd.com> wrote:
>
> [AMD Official Use Only - General]
>
> Hello Peter,
>
> >> +bool iommu_sev_snp_supported(void)
> >> +{
> >> +       struct amd_iommu *iommu;
> >> +
> >> +       /*
> >> +        * The SEV-SNP support requires that IOMMU must be enabled, and is
> >> +        * not configured in the passthrough mode.
> >> +        */
> >> +       if (no_iommu || iommu_default_passthrough()) {
> >> +               pr_err("SEV-SNP: IOMMU is either disabled or
> >> + configured in passthrough mode.\n");
>
> > Like below could this say something like snp support is disabled because of iommu settings.
>
> Here we may need to be more precise with the error information indicating why SNP is not enabled.
> Please note that this patch may actually become part of the IOMMU + SNP patch series, where
> additional checks are done, for example, not enabling SNP if IOMMU v2 page tables are enabled,
> so precise error information will be useful here.

I agree we should be more precise. I just thought we should explicitly
state something like: "SEV-SNP: IOMMU is either disabled or configured
in passthrough mode, SNP cannot be supported".

On 6/22/2022 12:50 AM, Peter Gonda wrote:
> On Tue, Jun 21, 2022 at 11:45 AM Kalra, Ashish <Ashish.Kalra@amd.com> wrote:
>>
>> [AMD Official Use Only - General]
>>
>> Hello Peter,
>>
>>>> +bool iommu_sev_snp_supported(void)
>>>> +{
>>>> +       struct amd_iommu *iommu;
>>>> +
>>>> +       /*
>>>> +        * The SEV-SNP support requires that IOMMU must be enabled, and is
>>>> +        * not configured in the passthrough mode.
>>>> +        */
>>>> +       if (no_iommu || iommu_default_passthrough()) {
>>>> +               pr_err("SEV-SNP: IOMMU is either disabled or
>>>> + configured in passthrough mode.\n");
>>
>>> Like below could this say something like snp support is disabled because of iommu settings.
>>
>> Here we may need to be more precise with the error information indicating why SNP is not enabled.
>> Please note that this patch may actually become part of the IOMMU + SNP patch series, where
>> additional checks are done, for example, not enabling SNP if IOMMU v2 page tables are enabled,
>> so precise error information will be useful here.
> 
> I agree we should be more precise. I just thought we should explicitly
> state something like: "SEV-SNP: IOMMU is either disabled or configured
> in passthrough mode, SNP cannot be supported".

I can update this in the other patch series here

https://lists.linuxfoundation.org/pipermail/iommu/2022-June/066399.html

Thank you,
Suravee

[AMD Official Use Only - General]

Hello Boris,

>> +bool iommu_sev_snp_supported(void)
>> +{
>> +	struct amd_iommu *iommu;
>> +
>> +	/*
>> +	 * The SEV-SNP support requires that IOMMU must be enabled, and is
>> +	 * not configured in the passthrough mode.
>> +	 */
>> +	if (no_iommu || iommu_default_passthrough()) {
>> +		pr_err("SEV-SNP: IOMMU is either disabled or configured in passthrough mode.\n");
>> +		return false;
>> +	}
>> +
>> +	/*
>> +	 * Iterate through all the IOMMUs and verify the SNPSup feature is
>> +	 * enabled.
>> +	 */
>> +	for_each_iommu(iommu) {
>> +		if (!iommu_feature(iommu, FEATURE_SNP)) {
>> +			pr_err("SNPSup is disabled (devid: %02x:%02x.%x)\n",
>> +			       PCI_BUS_NUM(iommu->devid), PCI_SLOT(iommu->devid),
>> +			       PCI_FUNC(iommu->devid));
>> +			return false;
>> +		}
>> +	}
>> +
>> +	return true;
>> +}
>> +EXPORT_SYMBOL_GPL(iommu_sev_snp_supported);

> Why is this function exported?

This function is required to ensure that IOMMU supports the SEV-SNP feature before enabling the SNP feature
and calling SNP_INIT. This IOMMU support check is done in the AMD IOMMU driver with the 
iommu_sev_snp_supported() function so it is exported by the IOMMU driver and called by sev module
later for SNP initialization in snp_rmptable_init(). 

Thanks,
Ashish

[AMD Official Use Only - General]

Hello Boris,

>> This function is required to ensure that IOMMU supports the SEV-SNP 
>> feature before enabling the SNP feature and calling SNP_INIT.
>> This IOMMU support check is done in the AMD IOMMU driver with the
>> iommu_sev_snp_supported() function so it is exported by the IOMMU 
>> driver and called by sev module

>What sev module?

I meant the kvm-amd module. 

>The call to iommu_sev_snp_supported() is done by snp_rmptable_init() which is in arch/x86/kernel/sev.c. AFAICT.

>And that is not a module. But function exports are done for modules.

>So that export looks superfluous.

Yes realized this is called in arch/x86/kernel/sev.c, so yes the export is not needed and will remove it.

Thanks,
Ashish

On Tue, Jun 21, 2022 at 11:50:59AM -0600, Peter Gonda wrote:
> On Tue, Jun 21, 2022 at 11:45 AM Kalra, Ashish <Ashish.Kalra@amd.com> wrote:
> >
> > [AMD Official Use Only - General]
> >
> > Hello Peter,
> >
> > >> +bool iommu_sev_snp_supported(void)
> > >> +{
> > >> +       struct amd_iommu *iommu;
> > >> +
> > >> +       /*
> > >> +        * The SEV-SNP support requires that IOMMU must be enabled, and is
> > >> +        * not configured in the passthrough mode.
> > >> +        */
> > >> +       if (no_iommu || iommu_default_passthrough()) {
> > >> +               pr_err("SEV-SNP: IOMMU is either disabled or
> > >> + configured in passthrough mode.\n");
> >
> > > Like below could this say something like snp support is disabled because of iommu settings.
> >
> > Here we may need to be more precise with the error information indicating why SNP is not enabled.
> > Please note that this patch may actually become part of the IOMMU + SNP patch series, where
> > additional checks are done, for example, not enabling SNP if IOMMU v2 page tables are enabled,
> > so precise error information will be useful here.
> 
> I agree we should be more precise. I just thought we should explicitly
> state something like: "SEV-SNP: IOMMU is either disabled or configured
> in passthrough mode, SNP cannot be supported".

It really should be, in order to have any practical use:

	if (no_iommu) {
		pr_err("SEV-SNP: IOMMU is disabled.\n");
		return false;
	}

	if (iommu_default_passthrough()) {
		pr_err("SEV-SNP: IOMMU is configured in passthrough mode.\n");
		return false;
	}

The comment is *completely* redundant, it absolutely does
not serve any sane purpose. It just tells what the code
already clearly stating.

The combo error message on the other hand leaves you to
the question "which one was it", and for that reason
combining the checks leaves you to a louse debugging
experience.

BR, Jarkko

[AMD Official Use Only - General]

Hello Jarkko,

>> 
>> It really should be, in order to have any practical use:
>> 
>> 	if (no_iommu) {
>> 		pr_err("SEV-SNP: IOMMU is disabled.\n");
>> 		return false;
>> 	}
>> 
>> 	if (iommu_default_passthrough()) {
>> 		pr_err("SEV-SNP: IOMMU is configured in passthrough mode.\n");
>> 		return false;
>> 	}
>> 
>> The comment is *completely* redundant, it absolutely does not serve 
>> any sane purpose. It just tells what the code already clearly stating.
>> 
>> The combo error message on the other hand leaves you to the question 
>> "which one was it", and for that reason combining the checks leaves 
>> you to a louse debugging experience.

>Also, are those really *errors*? That implies that there is something wrong.

>Since you can have a legit configuration, IMHO they should be either warn or info. What do you think?

>They are definitely not errors

Yes, they can be warn or info, but as I mentioned above this patch is now part of IOMMU + SNP series,
so these comments are now relevant for that.

Thanks,
Ashish

On Fri, Aug 26, 2022 at 06:54:16PM +0000, Kalra, Ashish wrote:
> [AMD Official Use Only - General]
> 
> Hello Jarkko,
> 
> >> 
> >> It really should be, in order to have any practical use:
> >> 
> >> 	if (no_iommu) {
> >> 		pr_err("SEV-SNP: IOMMU is disabled.\n");
> >> 		return false;
> >> 	}
> >> 
> >> 	if (iommu_default_passthrough()) {
> >> 		pr_err("SEV-SNP: IOMMU is configured in passthrough mode.\n");
> >> 		return false;
> >> 	}
> >> 
> >> The comment is *completely* redundant, it absolutely does not serve 
> >> any sane purpose. It just tells what the code already clearly stating.
> >> 
> >> The combo error message on the other hand leaves you to the question 
> >> "which one was it", and for that reason combining the checks leaves 
> >> you to a louse debugging experience.
> 
> >Also, are those really *errors*? That implies that there is something wrong.
> 
> >Since you can have a legit configuration, IMHO they should be either warn or info. What do you think?
> 
> >They are definitely not errors
> 
> Yes, they can be warn or info, but as I mentioned above this patch is now part of IOMMU + SNP series,
> so these comments are now relevant for that.

Yeah, warn/info/error is less relevant than the
second point I was making.

It's a good idea to spit out two instead of one
to make best of spitting out anything in the first
place :-) That way you make no mistake interpreting
what does the log message connect to, which can
sometimes make a difference while debugging a
kernel issue.

BR, Jarkko