Re: [PATCH 03/24] iommu/amd: Detect and initialize AMD vIOMMU feature

Vasant Hegde <[email protected]>
Newsgroups dev.linux.lists.iommu,org.kernel.vger.linux-kernel
Message-ID <[email protected]>
Hi Suravee,

On 7/27/2026 6:58 PM, Suravee Suthikulpanit wrote:
> The feature is advertised w/ EFR[VIOMMUSup]. Please see the AMD IOMMU
> specification[1] for more detail.
> 
> Introduce a new global variable amd_iommu_viommu, which is used to
> control the feature enablement in the driver. Currently, the feature
> is default to disabled. Once the feature is fully supported, it will be
> changed to enabled by default.
> 
> [1] https://docs.amd.com/v/u/en-US/48882_3.11_IOMMU_PUB
> 
> Signed-off-by: Suravee Suthikulpanit <[email protected]>
> ---
>  drivers/iommu/amd/Makefile          |  2 +-
>  drivers/iommu/amd/amd_iommu.h       |  2 ++
>  drivers/iommu/amd/amd_iommu_types.h |  1 +
>  drivers/iommu/amd/amd_viommu.h      | 22 ++++++++++++++++++++++
>  drivers/iommu/amd/init.c            | 15 +++++++++++++++
>  drivers/iommu/amd/viommu.c          | 29 +++++++++++++++++++++++++++++
>  6 files changed, 70 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/iommu/amd/amd_viommu.h
>  create mode 100644 drivers/iommu/amd/viommu.c
> 
> diff --git a/drivers/iommu/amd/Makefile b/drivers/iommu/amd/Makefile


.../...

> --- a/drivers/iommu/amd/init.c
> +++ b/drivers/iommu/amd/init.c
> @@ -34,6 +34,7 @@
>  #include <linux/crash_dump.h>
>  
>  #include "amd_iommu.h"
> +#include "amd_viommu.h"
>  #include "../irq_remapping.h"
>  #include "../iommu-pages.h"
>  
> @@ -196,6 +197,9 @@ bool amdr_ivrs_remap_support __read_mostly;
>  
>  bool amd_iommu_force_isolation __read_mostly;
>  
> +/* VIOMMU enabling flag */
> +bool amd_iommu_viommu;

may be add "__ro_after_init" ?

> +
>  unsigned long amd_iommu_pgsize_bitmap __ro_after_init = AMD_IOMMU_PGSIZES;
>  
>  enum iommu_init_state {
> @@ -2188,6 +2192,12 @@ static int __init iommu_init_pci(struct amd_iommu *iommu)
>  	if (check_feature(FEATURE_PPR) && amd_iommu_alloc_ppr_log(iommu))
>  		return -ENOMEM;
>  
> +	ret = amd_viommu_init(iommu);
> +	if (ret) {
> +		pr_err("Failed to initialize vIOMMU.\n");
> +		amd_iommu_viommu = false;
> +	}
> +
>  	if (iommu->cap & (1UL << IOMMU_CAP_NPCACHE)) {
>  		pr_info("Using strict mode due to virtualization\n");
>  		iommu_set_dma_strict();
> @@ -2281,6 +2291,9 @@ static void print_iommu_info(void)
>  		if (check_feature2(FEATURE_SEVSNPIO_SUP))
>  			pr_cont(" SEV-TIO");
>  
> +		if (check_feature(FEATURE_VIOMMU))
> +			pr_cont(" vIOMMU");
> +
>  		pr_cont("\n");
>  	}
>  
> @@ -2293,6 +2306,8 @@ static void print_iommu_info(void)
>  		pr_info("V2 page table enabled (Paging mode : %d level)\n",
>  			amd_iommu_gpt_level);
>  	}
> +	if (amd_iommu_viommu)
> +		pr_info("AMD-Vi: vIOMMU enabled\n");

pr_fmt already prepends "AMD-Vi:". Please remove it from above pr_info.




>  }
>  
>  static int __init amd_iommu_init_pci(void)
> diff --git a/drivers/iommu/amd/viommu.c b/drivers/iommu/amd/viommu.c
> new file mode 100644
> index 000000000000..f4b5f96d4785
> --- /dev/null
> +++ b/drivers/iommu/amd/viommu.c
> @@ -0,0 +1,29 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * Copyright (C) 2023 Advanced Micro Devices, Inc.
> + */
> +
> +#define pr_fmt(fmt)     "AMD-Vi: " fmt
> +#define dev_fmt(fmt)    pr_fmt(fmt)
> +
> +#include <linux/iommu.h>
> +#include <linux/iommufd.h>
> +#include <linux/amd-iommu.h>
> +#include <uapi/linux/iommufd.h>
> +
> +#include <asm/iommu.h>
> +#include <asm/set_memory.h>
> +
> +#include "iommufd.h"
> +#include "amd_iommu.h"
> +#include "amd_iommu_types.h"
> +#include "amd_viommu.h"
> +
> +int __init amd_viommu_init(struct amd_iommu *iommu)
> +{
> +	if (!amd_iommu_viommu ||
> +	    !check_feature(FEATURE_VIOMMU))

If check_feature returns false, then return -ENOSUPPORT so that called sets
amd_iommu_viommu to false.

Also how about renaming it as "amd_iommu_viommu_present" ? Because following
patches defines another structure with same name. It works, but add confusion.

-Vasant
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.