| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 21 Jun 2017 12:09:45 +0530
From: Geetha Akula <geethasowjanya.akula@...il.com>
To: Will Deacon <will.deacon@....com>
Cc: Geetha sowjanya <gakula@...iumnetworks.com>,
Robin Murphy <robin.murphy@....com>,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Hanjun Guo <hanjun.guo@...aro.org>,
Sudeep Holla <sudeep.holla@....com>,
Linux IOMMU <iommu@...ts.linux-foundation.org>,
Robert Moore <robert.moore@...el.com>,
Lv Zheng <lv.zheng@...el.com>,
"Rafael J. Wysocki" <rjw@...ysocki.net>, jcm@...hat.com,
linux-kernel@...r.kernel.org,
Robert Richter <robert.richter@...ium.com>,
Catalin Marinas <catalin.marinas@....com>,
Sunil Goutham <sgoutham@...ium.com>,
linux-arm-kernel@...ts.infradead.org, linux-acpi@...r.kernel.org,
devel@...ica.org, Linu Cherian <linu.cherian@...ium.com>,
Charles Garcia-Tobin <Charles.Garcia-Tobin@....com>,
Rob Herring <robh@...nel.org>,
Geetha Sowjanya <geethasowjanya.akula@...ium.com>
Subject: Re: [PATCH v8 3/3] iommu/arm-smmu-v3: Add workaround for Cavium
ThunderX2 erratum #126
Hi Will,
On Tue, Jun 20, 2017 at 11:30 PM, Will Deacon <will.deacon@....com> wrote:
> On Tue, Jun 20, 2017 at 07:47:39PM +0530, Geetha sowjanya wrote:
>> From: Geetha Sowjanya <geethasowjanya.akula@...ium.com>
>>
>> Cavium ThunderX2 SMMU doesn't support MSI and also doesn't have unique irq
>> lines for gerror, eventq and cmdq-sync.
>>
>> SHARED_IRQ option is set as a errata workaround, which allows to share the irq
>> line by register single irq handler for all the interrupts.
>>
>> Signed-off-by: Geetha sowjanya <gakula@...iumnetworks.com>
>> ---
>> .../devicetree/bindings/iommu/arm,smmu-v3.txt | 5 ++
>> drivers/iommu/arm-smmu-v3.c | 73 ++++++++++++++++----
>> 2 files changed, 64 insertions(+), 14 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>> index 6ecc48c..44b40e0 100644
>> --- a/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>> +++ b/Documentation/devicetree/bindings/iommu/arm,smmu-v3.txt
>> @@ -55,6 +55,11 @@ the PCIe specification.
>> Set for Caviun ThunderX2 silicon that doesn't support
>> SMMU page1 register space.
>>
>> +- cavium,cn9900-broken-unique-irqline
>> + : Use single irq line for all the SMMUv3 interrupts.
>> + Set for Caviun ThunderX2 silicon that doesn't support
>> + MSI and also doesn't have unique irq lines for gerror,
>> + eventq and cmdq-sync.
>
> I think we're better off just supporting a new (optional) named interrupt
> as "combined", and then allowing that to be used instead of the others.
Are you suggesting to have new name irq "combined" like gerror ?
If yes, then this won't be possible with apci. We need to update iort spec to
add new name irq.
>
>> ** Example
>>
>> smmu@...00000 {
>> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
>> index 2dea4a9..6c0c632 100644
>> --- a/drivers/iommu/arm-smmu-v3.c
>> +++ b/drivers/iommu/arm-smmu-v3.c
>> @@ -598,6 +598,7 @@ struct arm_smmu_device {
>>
>> #define ARM_SMMU_OPT_SKIP_PREFETCH (1 << 0)
>> #define ARM_SMMU_OPT_PAGE0_REGS_ONLY (1 << 1)
>> +#define ARM_SMMU_OPT_SHARED_IRQ (1 << 2)
>
> Please call this COMBINED instead of SHARED (similarly elsewhere). That
> said, not sure we need this.
>
>> u32 options;
>>
>> struct arm_smmu_cmdq cmdq;
>> @@ -665,6 +666,7 @@ struct arm_smmu_option_prop {
>> static struct arm_smmu_option_prop arm_smmu_options[] = {
>> { ARM_SMMU_OPT_SKIP_PREFETCH, "hisilicon,broken-prefetch-cmd" },
>> { ARM_SMMU_OPT_PAGE0_REGS_ONLY, "cavium,cn9900-broken-page1-regspace"},
>> + { ARM_SMMU_OPT_SHARED_IRQ, "cavium,cn9900-broken-unique-irqline"},
>> { 0, NULL},
>> };
>>
>> @@ -1313,6 +1315,21 @@ static irqreturn_t arm_smmu_gerror_handler(int irq, void *dev)
>> writel(gerror, smmu->base + ARM_SMMU_GERRORN);
>> return IRQ_HANDLED;
>> }
>> +/* Shared irq handler*/
>> +static irqreturn_t arm_smmu_shared_irq_thread(int irq, void *dev)
>> +{
>> + struct arm_smmu_device *smmu = dev;
>> + irqreturn_t ret;
>> +
>> + ret = arm_smmu_gerror_handler(irq, dev);
>> + if (ret == IRQ_NONE) {
>> + arm_smmu_evtq_thread(irq, dev);
>> + arm_smmu_cmdq_sync_handler(irq, dev);
>> + if (smmu->features & ARM_SMMU_FEAT_PRI)
>> + arm_smmu_priq_thread(irq, dev);
>> + }
>> + return IRQ_HANDLED;
>> +}
>
> This isn't quite right, because you're now running low-level handlers (like
> the gerror handler) in threaded context. You're better off registering a
> low-level handler too (see below) which can kick gerror and cmdq_sync
> before unconditionally returning IRQ_WAKE_THREAD.
+static irqreturn_t arm_smmu_combined_irq_thread(int irq, void *dev)
+{
+ struct arm_smmu_device *smmu = dev;
+
+ arm_smmu_evtq_thread(irq, dev);
+ if (smmu->features & ARM_SMMU_FEAT_PRI)
+ arm_smmu_priq_thread(irq, dev);
+
+ return IRQ_HANDLED;
+}
+
+static irqreturn_t arm_smmu_combined_irq_handler(int irq, void *dev)
+{
+ irqreturn_t ret;
+
+ ret = arm_smmu_gerror_handler(irq, dev);
+ if (ret == IRQ_NONE) {
+ arm_smmu_cmdq_sync_handler(irq, dev);
+ return IRQ_WAKE_THREAD;
+ }
+ else
+ return ret;
+}
>
>>
>> /* IO_PGTABLE API */
>> static void __arm_smmu_tlb_sync(struct arm_smmu_device *smmu)
>> @@ -2230,18 +2247,9 @@ static void arm_smmu_setup_msis(struct arm_smmu_device *smmu)
>> devm_add_action(dev, arm_smmu_free_msis, dev);
>> }
>>
>> -static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>> +static void arm_smmu_setup_unique_irqs(struct arm_smmu_device *smmu)
>> {
>> - int ret, irq;
>> - u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>> -
>> - /* Disable IRQs first */
>> - ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
>> - ARM_SMMU_IRQ_CTRLACK);
>> - if (ret) {
>> - dev_err(smmu->dev, "failed to disable irqs\n");
>> - return ret;
>> - }
>> + int irq, ret;
>>
>> arm_smmu_setup_msis(smmu);
>>
>> @@ -2284,10 +2292,46 @@ static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>> if (ret < 0)
>> dev_warn(smmu->dev,
>> "failed to enable priq irq\n");
>> - else
>> - irqen_flags |= IRQ_CTRL_PRIQ_IRQEN;
>> }
>> }
>> +}
>> +
>> +static void arm_smmu_setup_shared_irqs(struct arm_smmu_device *smmu)
>> +{
>> + int ret, irq;
>> +
>> + /* Single irq is used for all queues, request single interrupt lines */
>> + irq = smmu->evtq.q.irq;
>> + if (irq) {
>> + ret = devm_request_threaded_irq(smmu->dev, irq, NULL,
>
> As above, stick your low-level handler in instead of NULL here.
>
>> + arm_smmu_shared_irq_thread,
>> + IRQF_ONESHOT | IRQF_SHARED,
>
> Why do you need IRQF_SHARED here?
+devm_request_threaded_irq(smmu->dev, irq,
+ arm_smmu_combined_irq_handler,
+ arm_smmu_combined_irq_thread,
+ IRQF_SHARED,
+ "arm-smmu-v3-combined-irq", smmu);
On multi-node system, node1 SMMU's share irq lines with node0 SMMU's.
>
>> + "arm-smmu-v3-shared_irq", smmu);
>
> Call this "combined" instead of shared, to avoid confusion with the IRQ
> flags.
>
>> + if (ret < 0)
>> + dev_warn(smmu->dev, "failed to enable shared irq\n");
>> + }
>> +}
>> +
>> +static int arm_smmu_setup_irqs(struct arm_smmu_device *smmu)
>> +{
>> + int ret;
>> + u32 irqen_flags = IRQ_CTRL_EVTQ_IRQEN | IRQ_CTRL_GERROR_IRQEN;
>> +
>> + /* Disable IRQs first */
>> + ret = arm_smmu_write_reg_sync(smmu, 0, ARM_SMMU_IRQ_CTRL,
>> + ARM_SMMU_IRQ_CTRLACK);
>> + if (ret) {
>> + dev_err(smmu->dev, "failed to disable irqs\n");
>> + return ret;
>> + }
>> +
>> + if (smmu->options & ARM_SMMU_OPT_SHARED_IRQ)
>> + arm_smmu_setup_shared_irqs(smmu);
>> + else
>> + arm_smmu_setup_unique_irqs(smmu);
>
> I'd rather just have something like:
>
> irq = platform_get_irq_byname(pdev, "combined");
>
> in the arm_smmu_device_probe function. If we find it's there, we use that
> in preference to the other interrupts.
>
> Will
Thanks,
Geetha.
Powered by blists - more mailing lists