Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751730AbdHQJdy (ORCPT ); Thu, 17 Aug 2017 05:33:54 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:46872 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751620AbdHQJdx (ORCPT ); Thu, 17 Aug 2017 05:33:53 -0400 Subject: Re: [PATCH] irqchip/gic-{v2m, v3-its}: check iommu capable before doing iommu map To: Marc Zyngier , Shawn Lin , Thomas Gleixner Cc: Jason Cooper , linux-kernel@vger.kernel.org, Joerg Roedel , iommu@lists.linux-foundation.org References: <1502958508-195670-1-git-send-email-shawn.lin@rock-chips.com> <8017a14e-0855-de2c-9a78-5abeab25aeb1@arm.com> <364d4b7a-052b-0a38-abe3-63af8f57d125@rock-chips.com> <7962097d-2f7a-47c2-ef1b-bbaeabf83db4@arm.com> From: Robin Murphy Message-ID: <561de7a7-ecfe-49f0-f001-9db286976481@arm.com> Date: Thu, 17 Aug 2017 10:33:50 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.2.1 MIME-Version: 1.0 In-Reply-To: <7962097d-2f7a-47c2-ef1b-bbaeabf83db4@arm.com> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5829 Lines: 136 On 17/08/17 10:22, Marc Zyngier wrote: > On 17/08/17 10:01, Shawn Lin wrote: >> Hi Marc >> >> On 2017/8/17 16:52, Marc Zyngier wrote: >>> On 17/08/17 09:28, Shawn Lin wrote: >>>> If a PCIe RC use gic-v2m or gic-v3-its as a msi domain but doesn't >>>> have iommu support, we don't need to do iommu_dma_map_msi_msg to >>>> get mapped iommu address as all we need is the physical address. >>>> Otherwise we see the oops shown below as we can't get a iommu_group >>>> for a device without iommu capable. >>>> >>>> Unable to handle kernel NULL pointer dereference at virtual address >>>> 000000d0 >>>> [00000000000000d0] user address but active_mm is swapper >>>> Internal error: Oops: 96000004 [#1] PREEMPT SMP >>>> Modules linked in: >>>> CPU: 3 PID: 1 Comm: swapper/0 Not tainted >>>> 4.13.0-rc5-next-20170815-00001-g0744890-dirty #53 >>>> Hardware name: Firefly-RK3399 Board (DT) >>>> task: ffff80007bc70000 task.stack: ffff80007bc6c000 >>>> PC is at iommu_get_domain_for_dev+0x38/0x50 >>>> LR is at iommu_dma_map_msi_msg+0x3c/0x1b8 >>>> pc : [] lr : [] pstate: >>>> a0000045 >>>> >>>> ... >>>> >>>> [] iommu_get_domain_for_dev+0x38/0x50 >>>> [] iommu_dma_map_msi_msg+0x3c/0x1b8 >>>> [] its_irq_compose_msi_msg+0x44/0x50 >>>> [] irq_chip_compose_msi_msg+0x40/0x58 >>>> [] msi_domain_activate+0x1c/0x48 >>>> [] __irq_domain_activate_irq+0x40/0x58 >>>> [] irq_domain_activate_irq+0x24/0x40 >>>> [] msi_domain_alloc_irqs+0x104/0x190 >>>> [] pci_msi_setup_msi_irqs+0x3c/0x48 >>>> [] __pci_enable_msi_range+0x21c/0x408 >>>> [] pci_alloc_irq_vectors_affinity+0x104/0x168 >>>> [] pcie_port_device_register+0x200/0x488 >>>> [] pcie_portdrv_probe+0x34/0xd0 >>>> [] local_pci_probe+0x3c/0xb8 >>>> [] pci_device_probe+0x138/0x170 >>>> [] driver_probe_device+0x21c/0x2d8 >>>> [] __device_attach_driver+0x9c/0xf8 >>>> [] bus_for_each_drv+0x5c/0x98 >>>> [] __device_attach+0xc4/0x138 >>>> [] device_attach+0x10/0x18 >>>> [] pci_bus_add_device+0x4c/0xa8 >>>> [] pci_bus_add_devices+0x44/0x90 >>>> [] rockchip_pcie_probe+0xc70/0xcc8 >>>> [] platform_drv_probe+0x58/0xc0 >>>> [] driver_probe_device+0x21c/0x2d8 >>>> [] __driver_attach+0xac/0xb0 >>>> [] bus_for_each_dev+0x64/0xa0 >>>> [] driver_attach+0x20/0x28 >>>> [] bus_add_driver+0x110/0x230 >>>> [] driver_register+0x60/0xf8 >>>> [] __platform_driver_register+0x40/0x48 >>>> [] rockchip_pcie_driver_init+0x18/0x20 >>>> [] do_one_initcall+0xb0/0x120 >>>> [] kernel_init_freeable+0x184/0x224 >>>> [] kernel_init+0x10/0x100 >>>> [] ret_from_fork+0x10/0x18 >>>> >>>> This patch is to fix the problem exposed by the commit mentioned below. >>>> Before this commit, iommu has a work around method to fix this but now >>>> it doesn't. So we could fix this in gic code but maybe still need a fixes >>>> tag here. >>>> >>>> Fixes: 05f80300dc8b ("iommu: Finish making iommu_group support mandatory") >>>> Signed-off-by: Shawn Lin >>> >>> That looks pretty horrible. Please see the patch I posted in response to >>> your initial report: >>> >>> http://marc.info/?l=linux-pci&m=150295809430903&w=2 >> >> Aha, I saw it but I didn't get your point of "I'm not convinced that >> playing with the group refcount in an irqchip is the right approach..." >> >> So, I'm not sure whether you prefer your patch? > I really dislike both: > > - yours: because it reinvent the wheel (parsing DT one more time on each > MSI message creation), and is DT specific while the rest of the code is > completely firmware agnostic (what about ACPI?). > > - mine: because it relies on an intimate knowledge of the internals of > the iommu stuff, which is not what was originally intended. > > My comment was an invitation to Joerg to speak his mind. > > The original intent of iommu_dma_map_msi_msg() was that it could silently > fail without exploding in your face. We can change that, but a minimum of > coordination wouldn't hurt. > > My personal preference would be to address this in the iommu layer, > restoring a non-exploding iommu_dma_map_msi_msg(): Indeed, but that still doesn't fix other breakages that simply haven't been found yet ;) Thanks for the reports - I see exactly what I've done here. Gimme a moment to write it up convincingly... Robin. > diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c > index 44eca1e3243f..5440eae21bea 100644 > --- a/drivers/iommu/dma-iommu.c > +++ b/drivers/iommu/dma-iommu.c > @@ -883,12 +883,18 @@ static struct iommu_dma_msi_page *iommu_dma_get_msi_page(struct device *dev, > void iommu_dma_map_msi_msg(int irq, struct msi_msg *msg) > { > struct device *dev = msi_desc_to_dev(irq_get_msi_desc(irq)); > - struct iommu_domain *domain = iommu_get_domain_for_dev(dev); > + struct iommu_domain *domain; > + struct iommu_group *group; > struct iommu_dma_cookie *cookie; > struct iommu_dma_msi_page *msi_page; > phys_addr_t msi_addr = (u64)msg->address_hi << 32 | msg->address_lo; > unsigned long flags; > > + group = iommu_group_get(dev); > + if (!group) > + return; > + domain = iommu_get_domain_for_dev(dev); > + iommu_group_put(group); > if (!domain || !domain->iova_cookie) > return; > > Comments welcome. > > M. >