Re: [PATCH v3] iommu/arm-smmu: Add support for MSI on SMMUv3

[Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>]

Hi Marc,

On Thu, Oct 08, 2015 at 03:52:00PM +0100, Marc Zyngier wrote:
> Despite being a platform device, the SMMUv3 is capable of signaling
> interrupts using MSIs. Hook it into the platform MSI framework and
> enjoy faults being reported in a new and exciting way.
> 
> Signed-off-by: Marc Zyngier <marc.zyngier-5wv7dgnIgG8@public.gmane.org>
> ---
> * From v2:
>   - MSI indexes as an enum
>   - Fixed stupid 16bit writes instead of 32bit
>   - Added devm callback to release MSIs on teardown
>   - Moved all the MSI setup to its own function
> 
>  drivers/iommu/arm-smmu-v3.c | 108 ++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 99 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 5b11b77..3f7f096 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -26,6 +26,7 @@
>  #include <linux/iommu.h>
>  #include <linux/iopoll.h>
>  #include <linux/module.h>
> +#include <linux/msi.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
> @@ -403,6 +404,12 @@ enum pri_resp {
>  	PRI_RESP_SUCC,
>  };
>  
> +enum msi_index {
> +	EVTQ_MSI_INDEX,
> +	GERROR_MSI_INDEX,
> +	PRIQ_MSI_INDEX,
> +};
> +
>  struct arm_smmu_cmdq_ent {
>  	/* Common fields */
>  	u8				opcode;
> @@ -2176,6 +2183,92 @@ static int arm_smmu_write_reg_sync(struct arm_smmu_device *smmu, u32 val,
>  					  1, ARM_SMMU_POLL_TIMEOUT_US);
>  }
>  
> +static void arm_smmu_free_msis(void *data)
> +{
> +	struct arm_smmu_device *smmu = data;
> +	platform_msi_domain_free_irqs(smmu->dev);

So the smmu structure here is also managed by devm. What guarantees that
it doesn't get freed before your callback is invoked?

Also, none of this compiles if PCI_MSI=n.

> +}
> +
> +static void arm_smmu_write_msi_msg(struct msi_desc *desc, struct msi_msg *msg)
> +{
> +	struct device *dev = msi_desc_to_dev(desc);
> +	struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +	phys_addr_t cfg0_offset, cfg1_offset, cfg2_offset;
> +	phys_addr_t doorbell;
> +
> +	switch (desc->platform.msi_index) {
> +	case EVTQ_MSI_INDEX:
> +		cfg0_offset = ARM_SMMU_EVTQ_IRQ_CFG0;
> +		cfg1_offset = ARM_SMMU_EVTQ_IRQ_CFG1;
> +		cfg2_offset = ARM_SMMU_EVTQ_IRQ_CFG2;
> +		break;
> +	case GERROR_MSI_INDEX:
> +		cfg0_offset = ARM_SMMU_GERROR_IRQ_CFG0;
> +		cfg1_offset = ARM_SMMU_GERROR_IRQ_CFG1;
> +		cfg2_offset = ARM_SMMU_GERROR_IRQ_CFG2;
> +		break;
> +	case PRIQ_MSI_INDEX:
> +		cfg0_offset = ARM_SMMU_PRIQ_IRQ_CFG0;
> +		cfg1_offset = ARM_SMMU_PRIQ_IRQ_CFG1;
> +		cfg2_offset = ARM_SMMU_PRIQ_IRQ_CFG2;
> +		break;
> +	default:		/* Unknown */
> +		return;
> +	}
> +
> +	doorbell = (((u64)msg->address_hi) << 32) | msg->address_lo;
> +	doorbell &= MSI_CFG0_ADDR_MASK << MSI_CFG0_ADDR_SHIFT;
> +
> +	writeq_relaxed(doorbell, smmu->base + cfg0_offset);
> +	writel_relaxed(msg->data, smmu->base + cfg1_offset);
> +	writel_relaxed(MSI_CFG2_MEMATTR_DEVICE_nGnRE,
> +		       smmu->base + cfg2_offset);

This looks like the wrong way around to me. Once we've set a non-zero
doorbell, the hardware will switch to using MSI, so there's a potential
race where it generates an interrupt before we've initialised the payload.

Will