* Re: [Patch Part1 V2 01/17] iommu/vt-d: use dedicated bitmap to track remapping entry allocation status
[not found] ` <1385715030-20553-2-git-send-email-jiang.liu@linux.intel.com>
@ 2013-12-02 1:38 ` Yijing Wang
0 siblings, 0 replies; 6+ messages in thread
From: Yijing Wang @ 2013-12-02 1:38 UTC (permalink / raw)
To: Jiang Liu, Yinghai Lu, Joerg Roedel, David Woodhouse,
Dan Williams, Vinod Koul, Ashok Raj
Cc: iommu, linux-pci, linux-kernel, dmaengine
Tested-and-reviewed-by: Yijing Wang <wangyijing@huawei.com>
On 2013/11/29 16:50, Jiang Liu wrote:
> Currently Intel interrupt remapping drivers uses the "present" flag bit
> in remapping entry to track whether an entry is allocated or not.
> It works as follow:
> 1) allocate a remapping entry and set its "present" flag bit to 1
> 2) compose other fields for the entry
> 3) update the remapping entry with the composed value
>
> The remapping hardware may access the entry between step 1 and step 3,
> which then obervers an entry with the "present" flag set but random
> values in all other fields.
>
> This patch introduces a dedicated bitmap to track remapping entry
> allocation status instead of sharing the "present" flag with hardware,
> thus eliminate the race window. It also simplifies the implementation.
>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> drivers/iommu/intel_irq_remapping.c | 51 +++++++++++++++++------------------
> include/linux/intel-iommu.h | 1 +
> 2 files changed, 25 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/iommu/intel_irq_remapping.c b/drivers/iommu/intel_irq_remapping.c
> index bab10b1..282d392 100644
> --- a/drivers/iommu/intel_irq_remapping.c
> +++ b/drivers/iommu/intel_irq_remapping.c
> @@ -72,7 +72,6 @@ static int alloc_irte(struct intel_iommu *iommu, int irq, u16 count)
> u16 index, start_index;
> unsigned int mask = 0;
> unsigned long flags;
> - int i;
>
> if (!count || !irq_iommu)
> return -1;
> @@ -96,32 +95,17 @@ static int alloc_irte(struct intel_iommu *iommu, int irq, u16 count)
> }
>
> raw_spin_lock_irqsave(&irq_2_ir_lock, flags);
> - do {
> - for (i = index; i < index + count; i++)
> - if (table->base[i].present)
> - break;
> - /* empty index found */
> - if (i == index + count)
> - break;
> -
> - index = (index + count) % INTR_REMAP_TABLE_ENTRIES;
> -
> - if (index == start_index) {
> - raw_spin_unlock_irqrestore(&irq_2_ir_lock, flags);
> - printk(KERN_ERR "can't allocate an IRTE\n");
> - return -1;
> - }
> - } while (1);
> -
> - for (i = index; i < index + count; i++)
> - table->base[i].present = 1;
> -
> - cfg->remapped = 1;
> - irq_iommu->iommu = iommu;
> - irq_iommu->irte_index = index;
> - irq_iommu->sub_handle = 0;
> - irq_iommu->irte_mask = mask;
> -
> + index = bitmap_find_free_region(table->bitmap,
> + INTR_REMAP_TABLE_ENTRIES, mask);
> + if (index < 0) {
> + printk(KERN_ERR "can't allocate an IRTE\n");
> + } else {
> + cfg->remapped = 1;
> + irq_iommu->iommu = iommu;
> + irq_iommu->irte_index = index;
> + irq_iommu->sub_handle = 0;
> + irq_iommu->irte_mask = mask;
> + }
> raw_spin_unlock_irqrestore(&irq_2_ir_lock, flags);
>
> return index;
> @@ -254,6 +238,8 @@ static int clear_entries(struct irq_2_iommu *irq_iommu)
> set_64bit(&entry->low, 0);
> set_64bit(&entry->high, 0);
> }
> + bitmap_release_region(iommu->ir_table->bitmap, index,
> + irq_iommu->irte_mask);
>
> return qi_flush_iec(iommu, index, irq_iommu->irte_mask);
> }
> @@ -453,6 +439,7 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu, int mode)
> {
> struct ir_table *ir_table;
> struct page *pages;
> + unsigned long *bitmap;
>
> ir_table = iommu->ir_table = kzalloc(sizeof(struct ir_table),
> GFP_ATOMIC);
> @@ -470,7 +457,17 @@ static int intel_setup_irq_remapping(struct intel_iommu *iommu, int mode)
> return -ENOMEM;
> }
>
> + bitmap = kcalloc(BITS_TO_LONGS(INTR_REMAP_TABLE_ENTRIES),
> + sizeof(long), GFP_ATOMIC);
> + if (bitmap == NULL) {
> + printk(KERN_ERR "failed to allocate bitmap\n");
> + __free_pages(pages, INTR_REMAP_PAGE_ORDER);
> + kfree(ir_table);
> + return -ENOMEM;
> + }
> +
> ir_table->base = page_address(pages);
> + ir_table->bitmap = bitmap;
>
> iommu_set_irq_remapping(iommu, mode);
> return 0;
> diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h
> index d380c5e..de1e5e9 100644
> --- a/include/linux/intel-iommu.h
> +++ b/include/linux/intel-iommu.h
> @@ -288,6 +288,7 @@ struct q_inval {
>
> struct ir_table {
> struct irte *base;
> + unsigned long *bitmap;
> };
> #endif
>
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch Part1 V2 02/17] iommu/vt-d: fix PCI device reference leakage on error recovery path
[not found] ` <1385715030-20553-3-git-send-email-jiang.liu@linux.intel.com>
@ 2013-12-02 1:40 ` Yijing Wang
0 siblings, 0 replies; 6+ messages in thread
From: Yijing Wang @ 2013-12-02 1:40 UTC (permalink / raw)
To: Jiang Liu, Yinghai Lu, Joerg Roedel, David Woodhouse,
Dan Williams, Vinod Koul, Ashok Raj
Cc: iommu, linux-pci, linux-kernel, dmaengine
Reviewed-by: Yijing Wang <wangyijing@huawei.com>
On 2013/11/29 16:50, Jiang Liu wrote:
> Function dmar_parse_dev_scope() should release the PCI device reference
> count gained in function dmar_parse_one_dev_scope() on error recovery,
> otherwise will cause PCI device object leakage.
>
> This patch also introduces dmar_free_dev_scope(), which will be used
> to support DMAR device hotplug.
>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> drivers/iommu/dmar.c | 15 +++++++++++++--
> include/linux/dmar.h | 1 +
> 2 files changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 8b452c9..f3043a2 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -72,6 +72,7 @@ static int __init dmar_parse_one_dev_scope(struct acpi_dmar_device_scope *scope,
> struct acpi_dmar_pci_path *path;
> int count;
>
> + *dev = NULL;
> bus = pci_find_bus(segment, scope->bus);
> path = (struct acpi_dmar_pci_path *)(scope + 1);
> count = (scope->length - sizeof(struct acpi_dmar_device_scope))
> @@ -100,7 +101,6 @@ static int __init dmar_parse_one_dev_scope(struct acpi_dmar_device_scope *scope,
> if (!pdev) {
> pr_warn("Device scope device [%04x:%02x:%02x.%02x] not found\n",
> segment, scope->bus, path->device, path->function);
> - *dev = NULL;
> return 0;
> }
> if ((scope->entry_type == ACPI_DMAR_SCOPE_TYPE_ENDPOINT && \
> @@ -151,7 +151,7 @@ int __init dmar_parse_dev_scope(void *start, void *end, int *cnt,
> ret = dmar_parse_one_dev_scope(scope,
> &(*devices)[index], segment);
> if (ret) {
> - kfree(*devices);
> + dmar_free_dev_scope(devices, cnt);
> return ret;
> }
> index ++;
> @@ -162,6 +162,17 @@ int __init dmar_parse_dev_scope(void *start, void *end, int *cnt,
> return 0;
> }
>
> +void dmar_free_dev_scope(struct pci_dev ***devices, int *cnt)
> +{
> + if (*devices && *cnt) {
> + while (--*cnt >= 0)
> + pci_dev_put((*devices)[*cnt]);
> + kfree(*devices);
> + *devices = NULL;
> + *cnt = 0;
> + }
> +}
> +
> /**
> * dmar_parse_one_drhd - parses exactly one DMA remapping hardware definition
> * structure which uniquely represent one DMA remapping hardware unit
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index b029d1a..8adfce0 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -159,6 +159,7 @@ extern int dmar_parse_one_rmrr(struct acpi_dmar_header *header);
> extern int dmar_parse_one_atsr(struct acpi_dmar_header *header);
> extern int dmar_parse_dev_scope(void *start, void *end, int *cnt,
> struct pci_dev ***devices, u16 segment);
> +extern void dmar_free_dev_scope(struct pci_dev ***devices, int *cnt);
> extern int intel_iommu_init(void);
> #else /* !CONFIG_INTEL_IOMMU: */
> static inline int intel_iommu_init(void) { return -ENODEV; }
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch Part1 V2 04/17] iommu/vt-d: fix resource leakage on error recovery path in iommu_init_domains()
[not found] ` <1385715030-20553-5-git-send-email-jiang.liu@linux.intel.com>
@ 2013-12-02 1:41 ` Yijing Wang
0 siblings, 0 replies; 6+ messages in thread
From: Yijing Wang @ 2013-12-02 1:41 UTC (permalink / raw)
To: Jiang Liu, Yinghai Lu, Joerg Roedel, David Woodhouse,
Dan Williams, Vinod Koul, Ashok Raj
Cc: iommu, linux-pci, linux-kernel, dmaengine
On 2013/11/29 16:50, Jiang Liu wrote:
> Release allocated resources on error recovery path in function
> iommu_init_domains().
>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> drivers/iommu/intel-iommu.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index b8e3b48..2398876 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1273,6 +1273,8 @@ static int iommu_init_domains(struct intel_iommu *iommu)
> GFP_KERNEL);
> if (!iommu->domains) {
> printk(KERN_ERR "Allocating domain array failed\n");
> + kfree(iommu->domain_ids);
> + iommu->domain_ids = NULL;
> return -ENOMEM;
> }
>
>
Acked-by: Yijing Wang <wangyijing@huawei.com>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch Part1 V2 07/17] iommu/vt-d. trivial: check suitable flag in function detect_intel_iommu()
[not found] ` <1385715030-20553-9-git-send-email-jiang.liu@linux.intel.com>
@ 2013-12-02 1:42 ` Yijing Wang
0 siblings, 0 replies; 6+ messages in thread
From: Yijing Wang @ 2013-12-02 1:42 UTC (permalink / raw)
To: Jiang Liu, Yinghai Lu, Joerg Roedel, David Woodhouse,
Dan Williams, Vinod Koul, Ashok Raj
Cc: iommu, linux-pci, linux-kernel, dmaengine
This patch is the same as the last.:)
On 2013/11/29 16:50, Jiang Liu wrote:
> Flag irq_remapping_enabled is only set by intel_enable_irq_remapping(),
> which is called after detect_intel_iommu(). So we should check flag
> disable_irq_remap instead of irq_remapping_enabled in function
> detect_intel_iommu().
>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> drivers/iommu/dmar.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index f3043a2..77a066b 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -561,7 +561,7 @@ int __init detect_intel_iommu(void)
>
> dmar = (struct acpi_table_dmar *) dmar_tbl;
>
> - if (ret && irq_remapping_enabled && cpu_has_x2apic &&
> + if (ret && !disable_irq_remap && cpu_has_x2apic &&
> dmar->flags & 0x1)
> pr_info("Queued invalidation will be enabled to support x2apic and Intr-remapping.\n");
>
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch Part1 V2 07/17] iommu/vt-d, trivial: check suitable flag in function detect_intel_iommu()
[not found] ` <1385715030-20553-8-git-send-email-jiang.liu@linux.intel.com>
@ 2013-12-02 1:44 ` Yijing Wang
0 siblings, 0 replies; 6+ messages in thread
From: Yijing Wang @ 2013-12-02 1:44 UTC (permalink / raw)
To: Jiang Liu, Yinghai Lu, Joerg Roedel, David Woodhouse,
Dan Williams, Vinod Koul, Ashok Raj
Cc: iommu, linux-pci, linux-kernel, dmaengine
On 2013/11/29 16:50, Jiang Liu wrote:
> Flag irq_remapping_enabled is only set by intel_enable_irq_remapping(),
> which is called after detect_intel_iommu(). So we should check flag
> disable_irq_remap instead of irq_remapping_enabled in function
> detect_intel_iommu().
>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> drivers/iommu/dmar.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index f3043a2..77a066b 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -561,7 +561,7 @@ int __init detect_intel_iommu(void)
>
> dmar = (struct acpi_table_dmar *) dmar_tbl;
>
> - if (ret && irq_remapping_enabled && cpu_has_x2apic &&
> + if (ret && !disable_irq_remap && cpu_has_x2apic &&
> dmar->flags & 0x1)
> pr_info("Queued invalidation will be enabled to support x2apic and Intr-remapping.\n");
>
>
Reviewed-by: Yijing Wang <wangyijing@huawei.com>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch Part1 V2 12/17] iommu/vt-d: fix invalid memory access when freeing DMAR irq
[not found] ` <1385715030-20553-14-git-send-email-jiang.liu@linux.intel.com>
@ 2013-12-02 1:47 ` Yijing Wang
0 siblings, 0 replies; 6+ messages in thread
From: Yijing Wang @ 2013-12-02 1:47 UTC (permalink / raw)
To: Jiang Liu, Yinghai Lu, Joerg Roedel, David Woodhouse,
Dan Williams, Vinod Koul, Ashok Raj
Cc: iommu, linux-pci, linux-kernel, dmaengine
Reviewed-by: Yijing Wang <wangyijing@huawei.com>
On 2013/11/29 16:50, Jiang Liu wrote:
> In function free_dmar_iommu(), it sets IRQ handler data to NULL
> before calling free_irq(), which will cause invalid memory access
> because free_irq() will access IRQ handler data when calling
> function dmar_msi_mask(). So only set IRQ handler data to NULL
> after calling free_irq().
>
> Sample stack dump:
> [ 13.094010] BUG: unable to handle kernel NULL pointer dereference at 0000000000000048
> [ 13.103215] IP: [<ffffffff810a97cd>] __lock_acquire+0x4d/0x12a0
> [ 13.110104] PGD 0
> [ 13.112614] Oops: 0000 [#1] SMP
> [ 13.116585] Modules linked in:
> [ 13.120260] CPU: 60 PID: 1 Comm: swapper/0 Tainted: G W 3.13.0-rc1-gerry+ #9
> [ 13.129367] Hardware name: Intel Corporation LH Pass ........../SVRBD-ROW_T, BIOS SE5C600.86B.99.99.x059.091020121352 09/10/2012
> [ 13.142555] task: ffff88042dd38010 ti: ffff88042dd32000 task.ti: ffff88042dd32000
> [ 13.151179] RIP: 0010:[<ffffffff810a97cd>] [<ffffffff810a97cd>] __lock_acquire+0x4d/0x12a0
> [ 13.160867] RSP: 0000:ffff88042dd33b78 EFLAGS: 00010046
> [ 13.166969] RAX: 0000000000000046 RBX: 0000000000000002 RCX: 0000000000000000
> [ 13.175122] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000000000000048
> [ 13.183274] RBP: ffff88042dd33bd8 R08: 0000000000000002 R09: 0000000000000001
> [ 13.191417] R10: 0000000000000000 R11: 0000000000000001 R12: ffff88042dd38010
> [ 13.199571] R13: 0000000000000000 R14: 0000000000000048 R15: 0000000000000000
> [ 13.207725] FS: 0000000000000000(0000) GS:ffff88103f200000(0000) knlGS:0000000000000000
> [ 13.217014] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [ 13.223596] CR2: 0000000000000048 CR3: 0000000001a0b000 CR4: 00000000000407e0
> [ 13.231747] Stack:
> [ 13.234160] 0000000000000004 0000000000000046 ffff88042dd33b98 ffffffff810a567d
> [ 13.243059] ffff88042dd33c08 ffffffff810bb14c ffffffff828995a0 0000000000000046
> [ 13.251969] 0000000000000000 0000000000000000 0000000000000002 0000000000000000
> [ 13.260862] Call Trace:
> [ 13.263775] [<ffffffff810a567d>] ? trace_hardirqs_off+0xd/0x10
> [ 13.270571] [<ffffffff810bb14c>] ? vprintk_emit+0x23c/0x570
> [ 13.277058] [<ffffffff810ab1e3>] lock_acquire+0x93/0x120
> [ 13.283269] [<ffffffff814623f7>] ? dmar_msi_mask+0x47/0x70
> [ 13.289677] [<ffffffff8156b449>] _raw_spin_lock_irqsave+0x49/0x90
> [ 13.296748] [<ffffffff814623f7>] ? dmar_msi_mask+0x47/0x70
> [ 13.303153] [<ffffffff814623f7>] dmar_msi_mask+0x47/0x70
> [ 13.309354] [<ffffffff810c0d93>] irq_shutdown+0x53/0x60
> [ 13.315467] [<ffffffff810bdd9d>] __free_irq+0x26d/0x280
> [ 13.321580] [<ffffffff810be920>] free_irq+0xf0/0x180
> [ 13.327395] [<ffffffff81466591>] free_dmar_iommu+0x271/0x2b0
> [ 13.333996] [<ffffffff810a947d>] ? trace_hardirqs_on+0xd/0x10
> [ 13.340696] [<ffffffff81461a17>] free_iommu+0x17/0x50
> [ 13.346597] [<ffffffff81dc75a5>] init_dmars+0x691/0x77a
> [ 13.352711] [<ffffffff81dc7afd>] intel_iommu_init+0x351/0x438
> [ 13.359400] [<ffffffff81d8a711>] ? iommu_setup+0x27d/0x27d
> [ 13.365806] [<ffffffff81d8a739>] pci_iommu_init+0x28/0x52
> [ 13.372114] [<ffffffff81000342>] do_one_initcall+0x122/0x180
> [ 13.378707] [<ffffffff81077738>] ? parse_args+0x1e8/0x320
> [ 13.385016] [<ffffffff81d850e8>] kernel_init_freeable+0x1e1/0x26c
> [ 13.392100] [<ffffffff81d84833>] ? do_early_param+0x88/0x88
> [ 13.398596] [<ffffffff8154f8b0>] ? rest_init+0xd0/0xd0
> [ 13.404614] [<ffffffff8154f8be>] kernel_init+0xe/0x130
> [ 13.410626] [<ffffffff81574d6c>] ret_from_fork+0x7c/0xb0
> [ 13.416829] [<ffffffff8154f8b0>] ? rest_init+0xd0/0xd0
> [ 13.422842] Code: ec 99 00 85 c0 8b 05 53 05 a5 00 41 0f 45 d8 85 c0 0f 84 ff 00 00 00 8b 05 99 f9 7e 01 49 89 fe 41 89 f7 85 c0 0f 84 03 01 00 00 <49> 8b 06 be 01 00 00 00 48 3d c0 0e 01 82 0f 44 de 41 83 ff 01
> [ 13.450191] RIP [<ffffffff810a97cd>] __lock_acquire+0x4d/0x12a0
> [ 13.458598] RSP <ffff88042dd33b78>
> [ 13.462671] CR2: 0000000000000048
> [ 13.466551] ---[ end trace c5bd26a37c81d760 ]---
>
> Signed-off-by: Jiang Liu <jiang.liu@linux.intel.com>
> ---
> drivers/iommu/intel-iommu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 0ec49da..426095e 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -1289,9 +1289,9 @@ void free_dmar_iommu(struct intel_iommu *iommu)
> iommu_disable_translation(iommu);
>
> if (iommu->irq) {
> - irq_set_handler_data(iommu->irq, NULL);
> /* This will mask the irq */
> free_irq(iommu->irq, iommu);
> + irq_set_handler_data(iommu->irq, NULL);
> destroy_irq(iommu->irq);
> }
>
>
--
Thanks!
Yijing
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-12-02 1:48 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1385715030-20553-1-git-send-email-jiang.liu@linux.intel.com>
[not found] ` <1385715030-20553-2-git-send-email-jiang.liu@linux.intel.com>
2013-12-02 1:38 ` [Patch Part1 V2 01/17] iommu/vt-d: use dedicated bitmap to track remapping entry allocation status Yijing Wang
[not found] ` <1385715030-20553-3-git-send-email-jiang.liu@linux.intel.com>
2013-12-02 1:40 ` [Patch Part1 V2 02/17] iommu/vt-d: fix PCI device reference leakage on error recovery path Yijing Wang
[not found] ` <1385715030-20553-5-git-send-email-jiang.liu@linux.intel.com>
2013-12-02 1:41 ` [Patch Part1 V2 04/17] iommu/vt-d: fix resource leakage on error recovery path in iommu_init_domains() Yijing Wang
[not found] ` <1385715030-20553-9-git-send-email-jiang.liu@linux.intel.com>
2013-12-02 1:42 ` [Patch Part1 V2 07/17] iommu/vt-d. trivial: check suitable flag in function detect_intel_iommu() Yijing Wang
[not found] ` <1385715030-20553-8-git-send-email-jiang.liu@linux.intel.com>
2013-12-02 1:44 ` [Patch Part1 V2 07/17] iommu/vt-d, " Yijing Wang
[not found] ` <1385715030-20553-14-git-send-email-jiang.liu@linux.intel.com>
2013-12-02 1:47 ` [Patch Part1 V2 12/17] iommu/vt-d: fix invalid memory access when freeing DMAR irq Yijing Wang
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).