iommu/arm-smmu: Add support for MSI on SMMUv3
PATCH v3 1/1 iommu/arm-smmu: Add support for MSI on SMMUv3
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.
---
* 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(-)
drivers/iommu/arm-smmu-v3.c +99 -9
@@ -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); +} + +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); +} + +static void arm_smmu_setup_msis (struct arm_smmu_device *smmu) +{ +/* Clear the MSI address regs */ +writeq_relaxed (0 , smmu->base +ARM_SMMU_GERROR_IRQ_CFG0 ); +writeq_relaxed (0 , smmu->base +ARM_SMMU_EVTQ_IRQ_CFG0 ); +if (smmu->features &ARM_SMMU_FEAT_PRI ) +writeq_relaxed (0 , smmu->base +ARM_SMMU_PRIQ_IRQ_CFG0 ); + +/* Allocate MSIs for evtq, gerror and priq. Ignore cmdq */ +if (smmu->features &ARM_SMMU_FEAT_MSI ) { +struct msi_desc *desc; +int ret, nvecs =2 ; + +if (smmu->features &ARM_SMMU_FEAT_PRI ) + nvecs++; + + ret =platform_msi_domain_alloc_irqs (smmu->dev, nvecs, + arm_smmu_write_msi_msg); +if (ret) { +dev_warn (smmu->dev,"failed to allocate MSIs\n" ); +return ; + } + +for_each_msi_entry (desc, smmu->dev) { +switch (desc->platform.msi_index) { +case EVTQ_MSI_INDEX : + smmu->evtq.q.irq = desc->irq; +break ; +case GERROR_MSI_INDEX : + smmu->gerr_irq = desc->irq; +break ; +case PRIQ_MSI_INDEX : + smmu->priq.q.irq = desc->irq; +break ; +default :/* Unknown */ +continue ; + } + } + +/* Add callback to free MSIs on teardown */ +devm_add_action (smmu->dev, arm_smmu_free_msis, smmu); + } +} +static int arm_smmu_setup_irqs (struct arm_smmu_device *smmu) {int ret, irq;
@@ -2189,11 +2282,9 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)return ret; } -/* Clear the MSI address regs */ -writeq_relaxed (0 , smmu->base +ARM_SMMU_GERROR_IRQ_CFG0 ); -writeq_relaxed (0 , smmu->base +ARM_SMMU_EVTQ_IRQ_CFG0 ); +arm_smmu_setup_msis (smmu); -/* Request wired interrupt lines */ +/* Request interrupt lines */ irq = smmu->evtq.q.irq;if (irq) { ret = devm_request_threaded_irq(smmu->dev, irq,
@@ -2222,8 +2313,6 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu) }if (smmu->features &ARM_SMMU_FEAT_PRI ) { -writeq_relaxed (0 , smmu->base +ARM_SMMU_PRIQ_IRQ_CFG0 ); - irq = smmu->priq.q.irq;if (irq) { ret = devm_request_threaded_irq(smmu->dev, irq,
@@ -2421,7 +2510,7 @@ static int arm_smmu_device_probe(struct arm_smmu_device *smmu)if (reg &IDR0_SEV ) smmu->features |=ARM_SMMU_FEAT_SEV ; -if (reg &IDR0_MSI ) +if (IS_ENABLED (CONFIG_GENERIC_MSI_IRQ_DOMAIN ) && (reg &IDR0_MSI )) smmu->features |=ARM_SMMU_FEAT_MSI ;if (reg &IDR0_HYP )
@@ -2602,13 +2691,14 @@ static int arm_smmu_device_dt_probe(struct platform_device *pdev)if (ret)return ret; +/* Record our private device structure */ +platform_set_drvdata (pdev, smmu); +/* Reset the device */ ret =arm_smmu_device_reset (smmu);if (ret) goto out_free_structures; -/* Record our private device structure */ -platform_set_drvdata (pdev, smmu);return 0 ; out_free_structures:
Signature · 1 line
2.1.4
PATCH v3 1/1 iommu/arm-smmu: Add support for MSI on SMMUv3
Hi Marc,
Quoted 41 lines · Marc Zyngier
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 <[email protected]>---
* 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 functiondrivers/iommu/arm-smmu-v3.c | 108 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 99 insertions(+), 9 deletions(-)drivers/iommu/arm-smmu-v3.c +7
-0@@ -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;
drivers/iommu/arm-smmu-v3.c static int arm_smmu_write_reg_sync(struct arm_smmu_device *smmu, u32 val, · line 2189
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c +4
-0@@ -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.
Quoted 26 lines
drivers/iommu/arm-smmu-v3.c +26
-0+} + +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 */
drivers/iommu/arm-smmu-v3.c static int arm_smmu_write_reg_sync(struct arm_smmu_device *smmu, u32 val, · line 2225
Quoted 10 lines
drivers/iommu/arm-smmu-v3.c +10
-0+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
PATCH v3 1/1 iommu/arm-smmu: Add support for MSI on SMMUv3
Quoted 47 lines · Will Deacon
Hi Marc,
Quoted 44 lines · Marc Zyngier
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 <[email protected]>---
* 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 functiondrivers/iommu/arm-smmu-v3.c | 108 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 99 insertions(+), 9 deletions(-)drivers/iommu/arm-smmu-v3.c +7
-0@@ -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); }
Quoted 7 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_write_reg_sync(struct arm_smmu_device *smmu, u32 val, · line 2189
Quoted 4 lines
drivers/iommu/arm-smmu-v3.c +4
-0+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?
Because the whole devm thing is managed as a stack (each allocation or action is pushed on the stack), and actions are popped off the stack on teardown. See add_dr/release_nodes. This guarantee that the smmu structure cannot be free before the MSIs are released.
Now, a good way to settle the matter would be to pass the device structure instead of the smmu, removing the dependency altogether.
Quoted 1 line
Also, none of this compiles if PCI_MSI=n.
Gahh. Obviously, we need to select GENERIC_MSI_IRQ_DOMAIN. I'll update this as well.
Quoted 32 lines
Quoted 32 lines
drivers/iommu/arm-smmu-v3.c +32
-0+} + +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 ; +
Quoted 8 lines
drivers/iommu/arm-smmu-v3.c static int arm_smmu_write_reg_sync(struct arm_smmu_device *smmu, u32 val, · line 2225
Quoted 4 lines
drivers/iommu/arm-smmu-v3.c +4
-0+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.
We should be fine: we start by disabling interrupts (which is the only sane way to update the MSI registers). It is only the end of arm_smmu_setup_irq that we enable interrupts, which makes sure that the hardware will not generate any MSI in the interval.
I'll update the above and repost.
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny...