Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751622AbdHQJW7 (ORCPT ); Thu, 17 Aug 2017 05:22:59 -0400 Received: from usa-sjc-mx-foss1.foss.arm.com ([217.140.101.70]:46768 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750913AbdHQJW5 (ORCPT ); Thu, 17 Aug 2017 05:22:57 -0400 Subject: Re: [PATCH] irqchip/gic-{v2m, v3-its}: check iommu capable before doing iommu map To: Shawn Lin , Thomas Gleixner Cc: Jason Cooper , linux-kernel@vger.kernel.org, Joerg Roedel , Robin Murphy , 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> From: Marc Zyngier Organization: ARM Ltd Message-ID: <7962097d-2f7a-47c2-ef1b-bbaeabf83db4@arm.com> Date: Thu, 17 Aug 2017 10:22:53 +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: <364d4b7a-052b-0a38-abe3-63af8f57d125@rock-chips.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: 5461 Lines: 128 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(): 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. -- Jazz is not dead. It just smells funny...