Re: [PATCH 5/9] iommu/vt-d: Use dmar_can_force_on() for platform optin

[Baolu Lu]

On 6/4/2026 1:15 PM, Kevin Tian wrote:
> So the policy of requesting ACS in detect_intel_iommu() is consistent
> with that in platform_optin_force_iommu().
>
> While at it, remove no_platform_optin which is unnecessary now.
>
> Signed-off-by: Kevin Tian <kevin.tian@xxxxxxxxx>
> ---
>   drivers/iommu/intel/iommu.c | 14 +++++++-------
>   1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 0fc131a34963..edf01261a41d 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -58,7 +58,6 @@ static int rwbf_quirk;
>    */
>   static int force_on = 0;
>   int intel_iommu_tboot_noforce;
> -static int no_platform_optin;
>   
>   #define ROOT_ENTRY_NR (VTD_PAGE_SIZE/sizeof(struct root_entry))
>   
> @@ -248,7 +247,6 @@ static int __init intel_iommu_setup(char *str)
>   		} else if (!strncmp(str, "off", 3)) {
>   			dmar_state = DMAR_DISABLED_USER;
>   			dmar_disabled = 1;
> -			no_platform_optin = 1;
>   			pr_info("IOMMU disabled\n");
>   		} else if (!strncmp(str, "igfx_off", 8)) {
>   			disable_igfx_iommu = 1;
> @@ -2486,20 +2484,22 @@ static bool has_external_pci(void)
>   
>   static int __init platform_optin_force_iommu(void)
>   {
> -	if (no_iommu || !dmar_platform_optin() || no_platform_optin ||
> -	    !has_external_pci())
> +	if (!dmar_platform_optin() || !dmar_can_force_on(DMAR_FORCEON_PLATFORM))
>   		return 0;
>   
> -	if (dmar_disabled)
> -		pr_info("Intel-IOMMU force enabled due to platform opt in\n");
> +	if (!has_external_pci())
> +		return 0;

Moving has_external_pci() down here seems to be an optimization. :-)

Scanning the PCI fabric for external-facing ports can be costly, and now
we could skip it entirely if platform force-on is unnecessary.

>   
>   	/*
>   	 * If Intel-IOMMU is disabled by default, we will apply identity
>   	 * map for all devices except those marked as being untrusted.
>   	 */
> -	if (dmar_disabled)
> +	if (dmar_is_disabled()) {
> +		pr_info("Intel-IOMMU force enabled due to platform opt in\n");
>   		iommu_set_default_passthrough(false);
> +	}
>   
> +	dmar_state = DMAR_ENABLED_FORCE;

I was initially wondering if we needed a lock to protect dmar_state
here, but since this runs during early boot in a single-threaded
environment (indicated by the __init prefix), concurrent access isn't a
concern.

However, changing dmar_state here does mean that any logic calling
dmar_is_enabled() before this point will see a "disabled" state, while
subsequent calls will see an "enabled" state.

While I don't see a real problem right now, this state volatility is
something we should document. Could we add a brief comment here to
clarify this intentional state upgrade?


>   	dmar_disabled = 0;
>   
>   	return 1;

The rest looks good to me.

Thanks,
baolu