Re: [PATCH] arm64: iommu: Refactoring common code.
On Mon, Mar 10, 2014 at 04:32:50PM +, Ritesh Harjani wrote: Hi Everyone, Please find the following patch as refactoring of the common code out from arch/arm/mm/dma-mapping.c to lib/iommu-helper.c This is just an initial version of patch to get more details and to know if this is how we want to plan refactoring iommu code to lib/iommu-helper. Please let me know the changes/suggestion which you think in this ? Taking out the common code of buffer allocation and mapping for iommu from arch/arm to lib/iommu-helper file. Signed-off-by: Ritesh Harjani ritesh.harj...@gmail.com --- arch/arm/mm/dma-mapping.c| 159 +--- include/linux/iommu-helper.h | 22 ++ lib/iommu-helper.c | 169 +++ 3 files changed, 210 insertions(+), 140 deletions(-) I haven't had the time to look at this patch but I think you should change the subject prefix to just arm (rather than arm64) and cc Russell King and the linux-arm-kernel mailing list. I'll have a look at the patch tomorrow. Thanks. -- Catalin ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCHv3 0/6] Crashdump Accepting Active IOMMU
Hi Joerg, On Tue 3/4/2014 9:06 AM, Joerg Roedel wrote: On Fri, Jan 10, 2014 at 03:07:26PM -0700, Bill Sumner wrote: Bill Sumner (6): Crashdump-Accepting-Active-IOMMU-Flags-and-Prototype Crashdump-Accepting-Active-IOMMU-Utility-functions Crashdump-Accepting-Active-IOMMU-Domain-Interfaces Crashdump-Accepting-Active-IOMMU-Copy-Translations Crashdump-Accepting-Active-IOMMU-Debug-Print-IOMMU Crashdump-Accepting-Active-IOMMU-Call-From-Mainline drivers/iommu/intel-iommu.c | 1293 --- 1 file changed, 1225 insertions(+), 68 deletions(-) This is a pretty big change, before merging I would like to get more eyes on it. Please make sure you get more reviews on the code and the general concept. A few things I noticed while looking at it: Yes, as I was developing this, I realized that it is far more code than most Linux submissions. In addition to more eyes reviewing the code, I hope to see additional members of the community trying it out -- testing it on a wide variety of system configurations. Let me encourage additional reviewers and testers to look at this fix to crashdump. * You put a lot of code into #define CONFIG_CRASH_DUMP. Isn't the same approach necessary to for KEXEC? Until I saw your email, I had been focused only on the immediate problem of fixing crashdump and had not considered using this approach in the more-general context of KEXEC. It certainly looks like it could be useful with KEXEC as well. I would like to take-on the effort of expanding this technique into KEXEC as a separate follow-on project so that the original fix crashdump project is not delayed. On a quick look, I believe that the amount of additional code would be minimal; but the expansion of the testing matrix might be quite large. * Please get rid of all the pr_debug stuff. Will do. * I think it makes sense to put the functions to access the IOMMU configuration values of the old kernel into a new file. This is better than adding over 1000 new lines to intel-iommu.c Sounds good. I will move these functions into a new file. * I have seen your new single-patch for this change. A single patch with more than 1200 changed lines is not something anyone is willing to review. Please split the patches again in a meaningful and bisectable way. I will return to a multiple-patch set for future submissions. -- Bill ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
RE: [PATCHv3 3/6] Crashdump-Accepting-Active-IOMMU-Domain-Interfaces
On Tue 3/4/2014 8:59 AM, Joerg Roedel wrote
On Fri, Jan 10, 2014 at 03:07:29PM -0700, Bill Sumner wrote:
+context_get_entry(struct context_entry *context_addr,
+struct intel_iommu *iommu, u32 bus, int devfn) {
+unsigned long long q; /* quadword scratch */
+struct root_entry *root_phys; /* Phys adr (root table entry) */
+struct root_entry root_temp; /* Local copy of root_entry */
+struct context_entry *context_phys; /* Phys adr */
+
+if (pr_dbg.domain_get)
+pr_debug(ENTER %s B:D:F=%2.2x:%2.2x:%1.1x
context_entry:0x%llx intel_iommu:0x%llx\n,
+__func__, bus, devfn3, devfn7,
+(u64)context_addr, (u64)iommu);
+
+if (bus 255) /* Sanity check */
+return -5;
+if (devfn 255 || devfn 0) /* Sanity check */
+return -6;
+
+q = readq(iommu-reg + DMAR_RTADDR_REG);
+pr_debug(IOMMU %d: DMAR_RTADDR_REG:0x%16.16llx\n, iommu-seq_id, q);
+if (!q)
+return -1;
+
+root_phys = (struct root_entry *) q;/* Adr(base of vector) */
+root_phys += bus; /* Adr(entry we want) */
+
+oldcopy(root_temp, root_phys, sizeof(root_temp));
+
+pr_debug(root_temp.val:0x%llx .rsvd1:0x%llx root_phys:0x%llx\n,
+root_temp.val, root_temp.rsvd1, (u64)root_phys);
+
+if (!root_present(root_temp))
+return -2;
+
+pr_debug(B:D:F=%2.2x:%2.2x:%1.1x root_temp.val: %llx .rsvd1: %llx\n,
+bus, devfn 3, devfn 7, root_temp.val, root_temp.rsvd1);
+
+if (root_temp.rsvd1)/* If (root_entry is bad) */
+return -3;
+
+context_phys = get_context_phys_from_root(root_temp);
+if (!context_phys)
+return -4;
What do all these magic return values mean?
They all mean No value to return. The different negative values are an
artifact of the development where I wanted to make it easy not only to tell
that something went wrong, but also to have enough detail to figure-out what
happened without having to add code and repeat the test. I will clean-up these
in the next submission -- along with a few additional places where there are
similar leftovers.
Bill
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCHv3 0/6] Crashdump Accepting Active IOMMU
On Fri, Jan 10, 2014 at 03:07:26PM -0700, Bill Sumner wrote: [..] This patch set modifies the behavior of the iommu in the (new) crashdump kernel: 1. to accept the iommu hardware in an active state, 2. to leave the current translations in-place so that legacy DMA will continue using its current buffers until the device drivers in the crashdump kernel initialize and initialize their devices, 3. to use different portions of the iova address ranges for the device drivers in the crashdump kernel than the iova ranges that were in-use at the time of the panic. Conceptually, above makes sense to me. I have few queries. - Do we need to pass any kind of data from first kernel to second kernel, like table size etc? Calgary IOMMU was using first kernel's tables also and it was determining previous kernel's table size using saved_max_pfn. - I don't know how IOMMU translation tables look like, but are new DMA zones setup by drivers in second kernel part of same table? How do we make sure that sufficient space is available. I am not sure if possible table corruption from crashed kernel is an issue here or not. In general, I think changelogs of these patches need to be little better. There seem to be lot of text and still I can't quickly wrap my head around what a particular patch is supposed to be doing. But we definitely need to fix this issue. IOMMU issues with kdump have been troubling us for a very long time. Thanks Vivek ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
