* [PATCH v3] iommu/arm-smmu: Add support for MSI on SMMUv3
@ 2015-10-08 14:52 Marc Zyngier
2015-10-13 15:41 ` Will Deacon
0 siblings, 1 reply; 3+ messages in thread
From: Marc Zyngier @ 2015-10-08 14:52 UTC (permalink / raw)
To: linux-arm-kernel
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@arm.com>
---
* 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);
+}
+
+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:
--
2.1.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* [PATCH v3] iommu/arm-smmu: Add support for MSI on SMMUv3
2015-10-08 14:52 [PATCH v3] iommu/arm-smmu: Add support for MSI on SMMUv3 Marc Zyngier
@ 2015-10-13 15:41 ` Will Deacon
2015-10-13 17:04 ` Marc Zyngier
0 siblings, 1 reply; 3+ messages in thread
From: Will Deacon @ 2015-10-13 15:41 UTC (permalink / raw)
To: linux-arm-kernel
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@arm.com>
> ---
> * 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH v3] iommu/arm-smmu: Add support for MSI on SMMUv3
2015-10-13 15:41 ` Will Deacon
@ 2015-10-13 17:04 ` Marc Zyngier
0 siblings, 0 replies; 3+ messages in thread
From: Marc Zyngier @ 2015-10-13 17:04 UTC (permalink / raw)
To: linux-arm-kernel
On 13/10/15 16:41, Will Deacon wrote:
> 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@arm.com>
>> ---
>> * 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?
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.
> 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.
>> +}
>> +
>> +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.
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.
--
Jazz is not dead. It just smells funny...
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-10-13 17:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-10-08 14:52 [PATCH v3] iommu/arm-smmu: Add support for MSI on SMMUv3 Marc Zyngier
2015-10-13 15:41 ` Will Deacon
2015-10-13 17:04 ` Marc Zyngier
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).