Re: [PATCH v2 07/11] iommu/arm-smmu-v3: Disable PRI when no IRQ handler is registered

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:

New issues:
- [High] Updating multiple CR0 enable bits in a single write causes CONSTRAINED UNPREDICTABLE behavior.

Pre-existing issues:
- [High] The kdump kernel path forcibly disables the PRI queue in hardware but leaves the ARM_SMMU_FEAT_PRI software flag set.
- [High] Failure to enable IRQ generation in hardware bypasses the PRI disabling logic, leaving the PRI queue enabled without functioning interrupts.
- [High] SMMU interrupts (EVTQ and GERROR) are enabled in hardware even if the OS fails to register their IRQ handlers, leading to potential unhandled interrupts.
--

commit cbf6d221eefa894b802ea8dddcf53abb97fab008
Author: Nicolin Chen <nicolinc@nvidia.com>

iommu/arm-smmu-v3: Disable PRI when no IRQ handler is registered

This patch updates the SMMUv3 driver to clear the ARM_SMMU_FEAT_PRI flag
when PRI queue IRQ setup fails. It also ensures that the PRI queue is
disabled in the hardware CR0 register if the feature is no longer supported
after IRQ setup.

> diff --git a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> index 371a8bbdf6756..72fd5caa27368 100644
> --- a/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> +++ b/drivers/iommu/arm/arm-smmu-v3/arm-smmu-v3.c
> @@ -4738,11 +4738,14 @@ static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>  							IRQF_ONESHOT,
>  							"arm-smmu-v3-priq",
>  							smmu);
> -			if (ret < 0)
> +			if (ret < 0) {
>  				dev_warn(smmu->dev,
>  					 "failed to enable priq irq\n");
> +				smmu->features &= ~ARM_SMMU_FEAT_PRI;
> +			}
>  		} else {
>  			dev_warn(smmu->dev, "no priq irq - PRI will be broken\n");
> +			smmu->features &= ~ARM_SMMU_FEAT_PRI;
>  		}
>  	}
>  }

[Severity: High]
This is a pre-existing issue, but EVTQ and GERROR interrupts are enabled in
the hardware IRQ_CTRL register even if their IRQ handlers fail to register
earlier in arm_smmu_setup_unique_irqs():

    irq = smmu->evtq.q.irq;
    if (irq) {
        ret = devm_request_threaded_irq(...);
        if (ret < 0)
            dev_warn(smmu->dev, "failed to enable evtq irq\n");

Does the unconditional propagation of EVTQ and GERROR enable bits to the
hardware trigger persistent unhandled IRQ storms if registration fails?

[ ... ]

> @@ -4771,8 +4774,10 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>  					arm_smmu_combined_irq_thread,
>  					IRQF_ONESHOT,
>  					"arm-smmu-v3-combined-irq", smmu);
> -		if (ret < 0)
> +		if (ret < 0) {
>  			dev_warn(smmu->dev, "failed to enable combined irq\n");
> +			smmu->features &= ~ARM_SMMU_FEAT_PRI;
> +		}
>  	} else
>  		arm_smmu_setup_unique_irqs(smmu);
>  

[Severity: High]
This is a pre-existing issue, but if arm_smmu_write_reg_sync() fails to enable
IRQ generation in hardware at the end of arm_smmu_setup_irqs(), the function
still returns 0:

    ret = arm_smmu_write_reg_sync(smmu, irqen_flags,
                                  ARM_SMMU_IRQ_CTRL, ARM_SMMU_IRQ_CTRLACK);
    if (ret)
        dev_warn(smmu->dev, "failed to enable irqs\n");
    
    return 0;

Does this silent fall-through bypass the PRI disabling logic in the caller
and leave the hardware PRI queues enabled without functioning interrupt
drainers, potentially causing unrecoverable PCIe bus stalls?

[ ... ]

> @@ -4931,6 +4936,10 @@ static int arm_smmu_device_reset(struct arm_smmu_device *smmu)
>  		return ret;
>  	}
>  
> +	/* arm_smmu_setup_irqs() might have unset the ARM_SMMU_FEAT_PRI */
> +	if (!(smmu->features & ARM_SMMU_FEAT_PRI))
> +		enables &= ~CR0_PRIQEN;
> +
>  	if (is_kdump_kernel())
>  		enables &= ~(CR0_EVTQEN | CR0_PRIQEN);

[Severity: High]
This isn't a bug introduced by this patch, but the kdump kernel path forcibly
disables the PRI queue in hardware here while leaving the ARM_SMMU_FEAT_PRI
software flag set.

Does bypassing the clearance of ARM_SMMU_FEAT_PRI force active endpoint
capabilities against disabled hardware queues, causing permanent SMMU aborts?

[Severity: High]
Further down in arm_smmu_device_reset(), the SMMU interface is enabled using
the updated enables mask:

    enables |= CR0_SMMUEN;
    ret = arm_smmu_write_reg_sync(smmu, enables, ARM_SMMU_CR0,
                                  ARM_SMMU_CR0ACK);

Does updating multiple CR0 enable bits in a single write cause CONSTRAINED
UNPREDICTABLE behavior?

By clearing CR0_PRIQEN above and setting CR0_SMMUEN simultaneously here,
might this trigger an unpredictable hardware state? Would sequenced
write-and-ACK patterns for each enable bit resolve this synchronization flaw?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1779944354.git.nicolinc@nvidia.com?part=7