Re: [PATCH] irqchip/gic-{v2m, v3-its}: check iommu capable before doing iommu map
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
00d0
[00d0] user address but active_mm is swapper
Internal error: Oops: 9604 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 PID: 1 Comm: swapper/0 Not tainted
4.13.0-rc5-next-20170815-1-g0744890-dirty #53
Hardware name: Firefly-RK3399 Board (DT)
task: 80007bc7 task.stack: 80007bc6c000
PC is at iommu_get_domain_for_dev+0x38/0x50
LR is at iommu_dma_map_msi_msg+0x3c/0x1b8
pc : [] lr : [] pstate:
a045
...
[] 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=150295809430903=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);
> +
Re: [PATCH] irqchip/gic-{v2m, v3-its}: check iommu capable before doing iommu map
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
>>> 00d0
>>> [00d0] user address but active_mm is swapper
>>> Internal error: Oops: 9604 [#1] PREEMPT SMP
>>> Modules linked in:
>>> CPU: 3 PID: 1 Comm: swapper/0 Not tainted
>>> 4.13.0-rc5-next-20170815-1-g0744890-dirty #53
>>> Hardware name: Firefly-RK3399 Board (DT)
>>> task: 80007bc7 task.stack: 80007bc6c000
>>> PC is at iommu_get_domain_for_dev+0x38/0x50
>>> LR is at iommu_dma_map_msi_msg+0x3c/0x1b8
>>> pc : [] lr : [] pstate:
>>> a045
>>>
>>> ...
>>>
>>> [] 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=150295809430903=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...
___
iommu mailing list
iommu@lists.linux-foundation.org
Re: [PATCH] irqchip/gic-{v2m, v3-its}: check iommu capable before doing iommu map
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
00d0
[00d0] user address but active_mm is swapper
Internal error: Oops: 9604 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 PID: 1 Comm: swapper/0 Not tainted
4.13.0-rc5-next-20170815-1-g0744890-dirty #53
Hardware name: Firefly-RK3399 Board (DT)
task: 80007bc7 task.stack: 80007bc6c000
PC is at iommu_get_domain_for_dev+0x38/0x50
LR is at iommu_dma_map_msi_msg+0x3c/0x1b8
pc : [] lr : [] pstate:
a045
...
[] 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=150295809430903=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?
Thanks,
M.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] irqchip/gic-{v2m, v3-its}: check iommu capable before doing iommu map
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
> 00d0
> [00d0] user address but active_mm is swapper
> Internal error: Oops: 9604 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 3 PID: 1 Comm: swapper/0 Not tainted
> 4.13.0-rc5-next-20170815-1-g0744890-dirty #53
> Hardware name: Firefly-RK3399 Board (DT)
> task: 80007bc7 task.stack: 80007bc6c000
> PC is at iommu_get_domain_for_dev+0x38/0x50
> LR is at iommu_dma_map_msi_msg+0x3c/0x1b8
> pc : [] lr : [] pstate:
> a045
>
> ...
>
> [] 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=150295809430903=2
Thanks,
M.
--
Jazz is not dead. It just smells funny...
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] irqchip/gic-{v2m, v3-its}: check iommu capable before doing iommu map
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
00d0
[00d0] user address but active_mm is swapper
Internal error: Oops: 9604 [#1] PREEMPT SMP
Modules linked in:
CPU: 3 PID: 1 Comm: swapper/0 Not tainted
4.13.0-rc5-next-20170815-1-g0744890-dirty #53
Hardware name: Firefly-RK3399 Board (DT)
task: 80007bc7 task.stack: 80007bc6c000
PC is at iommu_get_domain_for_dev+0x38/0x50
LR is at iommu_dma_map_msi_msg+0x3c/0x1b8
pc : [] lr : [] pstate:
a045
...
[] 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
---
drivers/irqchip/irq-gic-common.c | 10 ++
drivers/irqchip/irq-gic-common.h | 2 +-
drivers/irqchip/irq-gic-v2m.c| 6 +-
drivers/irqchip/irq-gic-v3-its.c | 4 +++-
4 files changed, 19 insertions(+), 3 deletions(-)
diff --git a/drivers/irqchip/irq-gic-common.c b/drivers/irqchip/irq-gic-common.c
index 9ae7180..c340d70 100644
--- a/drivers/irqchip/irq-gic-common.c
+++ b/drivers/irqchip/irq-gic-common.c
@@ -14,6 +14,7 @@
* along with this program. If not, see
