Re: [PATCH v11 04/10] iommu/vt-d: functions to copy data from old mem
Hi Baoquan, I am using a list here to store all the mapped addresses, and unmap them out of iounmap. About the reason, please check the old mails. I cannot remember the detailed reasons. Thanks Zhenhua On 05/13/2015 05:00 PM, Baoquan He wrote: On 05/11/15 at 05:52pm, Li, Zhen-Hua wrote: Add some functions to copy the data from old kernel. These functions are used to copy context tables and page tables. To avoid calling iounmap between spin_lock_irqsave and spin_unlock_irqrestore, use a link here, store the pointers , and then use iounmap to free them in another place. Hi Zhenhua, I remember you mentioned iounmap will cause error inside spin_lock_irqsave. Do you know why it happened now? And could you also describe why avoid calling iounmap between spin_lock_irqsave/unlock_irqsave is needed here and what's the status now? I think other reviewer may want to know it too. Thanks Baoquan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 04/10] iommu/vt-d: functions to copy data from old mem
On 05/13/15 at 05:13pm, Li, ZhenHua wrote: Hi Baoquan, I am using a list here to store all the mapped addresses, and unmap them out of iounmap. About the reason, please check the old mails. I cannot remember the detailed reasons. Yeah, I understand that the list is used to collect all mapped addresses and unmap them all after spin_unlock_irqsave. that's why it's better to put the reason in patch log. Patch log is used to record this kind of explanation so that you needn't say it again and again. My personal opinion. Thanks Baoquan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 06/10] iommu/vt-d: datatypes and functions used for kdump
On 05/11/15 at 05:52pm, Li, Zhen-Hua wrote:
Populate it with support functions to copy iommu translation tables from
from the panicked kernel into the kdump kernel in the event of a crash.
Functions:
Use old root entry table, and load the old data to root_entry as cache.
Malloc new context table and copy old context table to the new one.
Bill Sumner:
Original version, the creation of the data types and functions.
Li, Zhenhua:
Create new function iommu_check_pre_te_status() to check status.
Update the caller of context_get_* and context_put*, use context_*
and context_set_* for replacement.
Update the name of the function that loads root entry table.
Use new function to copy old context entry tables and page tables.
Use unsigned long for physical address.
Remove the functions to copy page table in Bill's version.
Remove usage of dve and ppap in Bill's version.
Signed-off-by: Bill Sumner billsumnerli...@gmail.com
Signed-off-by: Li, Zhen-Hua zhen-h...@hp.com
---
drivers/iommu/intel-iommu.c | 121
include/linux/intel-iommu.h | 3 ++
2 files changed, 124 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 3a5d446..28c3c64 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -386,6 +386,18 @@ struct iommu_remapped_entry {
static LIST_HEAD(__iommu_remapped_mem);
static DEFINE_MUTEX(__iommu_mem_list_lock);
+/*
+ * Copy iommu translation tables from old kernel into new kernel.
+ * Entry to this set of functions is: intel_iommu_load_translation_tables()
+ *
+ */
+
+static int copy_root_entry_table(struct intel_iommu *iommu);
+
+static int intel_iommu_load_translation_tables(struct intel_iommu *iommu);
+
+static void iommu_check_pre_te_status(struct intel_iommu *iommu);
+
/*
* This domain is a statically identity mapping domain.
* 1. This domain creats a static 1:1 mapping to all usable memory.
@@ -4987,3 +4999,112 @@ static void __iommu_update_old_root_entry(struct
intel_iommu *iommu, int index)
__iommu_flush_cache(iommu, to + start, size);
}
+/*
+ * Load root entry tables from old kernel.
+ */
+static int copy_root_entry_table(struct intel_iommu *iommu)
+{
+ u32 bus;/* Index: root-entry-table */
+ struct root_entry *re; /* Virt(iterator: new table) */
+ unsigned long context_old_phys; /* Phys(context table entry) */
+ struct context_entry *context_new_virt; /* Virt(new context_entry) */
+
+ /*
+ * A new root entry table has been allocated ,
+ * we need copy re from old kernel to the new allocated one.
+ */
+
+ if (!iommu-root_entry_old_phys)
+ return -ENOMEM;
+
+ for (bus = 0, re = iommu-root_entry; bus 256; bus += 1, re += 1) {
+ if (!root_present(re))
+ continue;
+
+ context_old_phys = get_context_phys_from_root(re);
+
+ if (!context_old_phys)
+ continue;
+
+ context_new_virt =
+ (struct context_entry *)alloc_pgtable_page(iommu-node);
+
+ if (!context_new_virt)
+ return -ENOMEM;
+
+ __iommu_load_from_oldmem(context_new_virt,
+ context_old_phys,
+ VTD_PAGE_SIZE);
+
+ __iommu_flush_cache(iommu, context_new_virt, VTD_PAGE_SIZE);
+
+ set_root_value(re, virt_to_phys(context_new_virt));
+ }
+
+ return 0;
+}
+
+/*
+ * Interface to the load translation tables set of functions
+ * from mainline code.
+ */
+static int intel_iommu_load_translation_tables(struct intel_iommu *iommu)
+{
+ unsigned long long q; /* quadword scratch */
+ int ret = 0;/* Integer return code */
+ unsigned long flags;
+
+ q = dmar_readq(iommu-reg + DMAR_RTADDR_REG);
+ if (!q)
+ return -1;
+
+ spin_lock_irqsave(iommu-lock, flags);
+
+ /* Load the root-entry table from the old kernel
+ * foreach context_entry_table in root_entry
+ * Copy each entry table from old kernel
+ */
+ if (!iommu-root_entry) {
+ iommu-root_entry =
+ (struct root_entry *)alloc_pgtable_page(iommu-node);
+ if (!iommu-root_entry) {
+ spin_unlock_irqrestore(iommu-lock, flags);
+ return -ENOMEM;
+ }
+ }
+
+ iommu-root_entry_old_phys = q VTD_PAGE_MASK;
+ if (!iommu-root_entry_old_phys) {
+ pr_err(Could not read old root entry address.);
+
Re: [PATCH v11 05/10] iommu/vt-d: Add functions to load and save old re
On 05/13/15 at 09:47am, Li, ZhenHua wrote: On 05/12/2015 04:37 PM, Dave Young wrote: Seems the subject was truncated? Maybe re means root entry? Then please fix it On 05/11/15 at 05:52pm, Li, Zhen-Hua wrote: Add functions to load root entry table from old kernel, and to save updated root entry table. Add two member in struct intel_iommu, to store the RTA in old kernel, and the mapped virt address of it. Please explain a bit what is RTA in patch log, it is unclear to most of people who do not know iommu details. If people want to understand this patchset, I assume they have read the vt-d specs. RTA is defined clearly in this spec. I think explain a bit is better, it will be easier for review, RTA is defined in spec, right, but if you refer to it in kernel source code, describe the meaning will help people to understand your code. I would not stick on this 'RTA', but a descriptive subject and patch log is still important, please check the logs, not only for this patch. Thanks Dave ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 02/10] iommu/vt-d: Items required for kdump
On 05/13/15 at 09:45am, Li, ZhenHua wrote: On 05/12/2015 04:17 PM, Dave Young wrote: On 05/11/15 at 05:52pm, Li, Zhen-Hua wrote: Add context entry functions needed for kdump. +/* + * Fix Crashdump failure caused by leftover DMA through a hardware IOMMU + * + * Fixes the crashdump kernel to deal with an active iommu and legacy + * DMA from the (old) panicked kernel in a manner similar to how legacy + * DMA is handled when no hardware iommu was in use by the old kernel -- + * allow the legacy DMA to continue into its current buffers. + * + * In the crashdump kernel, this code: + * 1. skips disabling the IOMMU's translating. + * 2. Do not re-enable IOMMU's translating. + * 3. In kdump kernel, use the old root entry table. + * 4. Allocate pages for new context entry, copy data from old context entries + *in the old kernel to the new ones. + * + * In other kinds of kernel, for example, a kernel started by kexec, + * do the same thing as crashdump kernel. + */ + + Above comments should come along with the code changes instead of putting it in this patch. BTW, there's one more blank line at the end.. Code change is in 00/10, the cover letter. I means the real code, not the changelog. And the blank does not matter, I checked the patch with scripts/checkpatch.pl, no warnings, no errors. Why add two line breaks if one is enough? Adding such check in checkpatch.pl make sense to me actually. Thanks Dave ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 04/10] iommu/vt-d: functions to copy data from old mem
On 05/11/15 at 05:52pm, Li, Zhen-Hua wrote: Add some functions to copy the data from old kernel. These functions are used to copy context tables and page tables. To avoid calling iounmap between spin_lock_irqsave and spin_unlock_irqrestore, use a link here, store the pointers , and then use iounmap to free them in another place. Hi Zhenhua, I remember you mentioned iounmap will cause error inside spin_lock_irqsave. Do you know why it happened now? And could you also describe why avoid calling iounmap between spin_lock_irqsave/unlock_irqsave is needed here and what's the status now? I think other reviewer may want to know it too. Thanks Baoquan ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 06/10] iommu/vt-d: datatypes and functions used for kdump
On 05/13/15 at 04:58pm, Li, ZhenHua wrote:
Hi Dave,
iommu-root_entry_old_virt is used to store the mapped old rta.
iommu-root_entry_old_phys is used to store the physical address
stored in registers.
So we must not free/unmap iommu-root_entry_old_phys .
Oh, yes. I was mistaken on this. Thanks for telling.
Zhenhua
On 05/13/2015 04:56 PM, Baoquan He wrote:
+
+ iommu-root_entry_old_phys = q VTD_PAGE_MASK;
+ if (!iommu-root_entry_old_phys) {
+ pr_err(Could not read old root entry address.);
+ return -1;
+ }
+
I didn't find where you call iounmap to free mapping of
iommu-root_entry_old_phys. Am I missing anything?
+ iommu-root_entry_old_virt = ioremap_cache(iommu-root_entry_old_phys,
+ VTD_PAGE_SIZE);
+ if (!iommu-root_entry_old_virt) {
+ pr_err(Could not map the old root entry.);
+ return -ENOMEM;
+ }
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 06/10] iommu/vt-d: datatypes and functions used for kdump
Hi Dave,
iommu-root_entry_old_virt is used to store the mapped old rta.
iommu-root_entry_old_phys is used to store the physical address stored
in registers.
So we must not free/unmap iommu-root_entry_old_phys .
Zhenhua
On 05/13/2015 04:56 PM, Baoquan He wrote:
+
+ iommu-root_entry_old_phys = q VTD_PAGE_MASK;
+ if (!iommu-root_entry_old_phys) {
+ pr_err(Could not read old root entry address.);
+ return -1;
+ }
+
I didn't find where you call iounmap to free mapping of
iommu-root_entry_old_phys. Am I missing anything?
+ iommu-root_entry_old_virt = ioremap_cache(iommu-root_entry_old_phys,
+ VTD_PAGE_SIZE);
+ if (!iommu-root_entry_old_virt) {
+ pr_err(Could not map the old root entry.);
+ return -ENOMEM;
+ }
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Any side effect if turn off amd_iommu
Hi AceLan Kao, On Wed, May 13, 2015 at 03:37:57PM +0800, AceLan Kao wrote: The method I described is not making any guess, we need a table to write down the correct ioapic value for those machines, we can add the BIOS version check(DMI_BIOS_VERSION) if you think the behavior may change with different version of BIOSes. You don't know which IO-APIC is which one without a correct IVRS table. AMD desktop systems have usually have up to two IO-APICs, one in the SB and one in the NB. But the ids of these IO-APICs are not fixed and can depend on the BIOS version as well as on the processor that is plugged into the board. So since we don't know which IO-APIC is which one we can't make any good guesses that work for everybody. When we implement workarounds we may fix it for one user, but risk breakage for others. If you still feel uncomfortable, I'm okay with any solutions, but I don't get it how to do it in your way. We can found those machines from bug reports(I have some machines on my hand, too), so we need a table to record those machines? And how to disable interrupt remapping, will it lead to any side effect? Disabling interrupt remapping has no big side effect. You only have to load vfio with a special parameter so allow unsafe interrupts to do device assignment with KVM. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v11 02/10] iommu/vt-d: Items required for kdump
On 05/12/2015 06:45 PM, Li, ZhenHua wrote: On 05/12/2015 04:17 PM, Dave Young wrote: On 05/11/15 at 05:52pm, Li, Zhen-Hua wrote: Add context entry functions needed for kdump. +/* + * Fix Crashdump failure caused by leftover DMA through a hardware IOMMU + * + * Fixes the crashdump kernel to deal with an active iommu and legacy + * DMA from the (old) panicked kernel in a manner similar to how legacy + * DMA is handled when no hardware iommu was in use by the old kernel -- + * allow the legacy DMA to continue into its current buffers. + * + * In the crashdump kernel, this code: + * 1. skips disabling the IOMMU's translating. + * 2. Do not re-enable IOMMU's translating. + * 3. In kdump kernel, use the old root entry table. + * 4. Allocate pages for new context entry, copy data from old context entries + *in the old kernel to the new ones. + * + * In other kinds of kernel, for example, a kernel started by kexec, + * do the same thing as crashdump kernel. + */ + + Above comments should come along with the code changes instead of putting it in this patch. BTW, there's one more blank line at the end.. Code change is in 00/10, the cover letter. And the blank does not matter, I checked the patch with scripts/checkpatch.pl, no warnings, no errors. I agree with Dave. The comment by itself makes no sense. You would be better off moving this to patch 3 where you actually start to add code. You remove the double space in patch 5 of your set so it must of triggered something. Neither of these are grounds for rejecting the patches, but it is something you may want to clean up if you end up resubmitting. - Alex ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: Any side effect if turn off amd_iommu
Hi Joerg, The method I described is not making any guess, we need a table to write down the correct ioapic value for those machines, we can add the BIOS version check(DMI_BIOS_VERSION) if you think the behavior may change with different version of BIOSes. If you still feel uncomfortable, I'm okay with any solutions, but I don't get it how to do it in your way. We can found those machines from bug reports(I have some machines on my hand, too), so we need a table to record those machines? And how to disable interrupt remapping, will it lead to any side effect? Best regards, AceLan Kao. 2015-05-05 23:13 GMT+08:00 Joerg Roedel j...@8bytes.org: Hi, On Tue, May 05, 2015 at 03:30:41PM +0800, AceLan Kao wrote: There is no easy way to check whether this BIOS bug is present, so we have to list them explicitly. I think we can do just like what platform drivers(ex. asus_quirks[1]) do, check the system vendor and product name to identify those machines, and give them a working ioapic value. Problem here is that system vendor and platform name is not enough, the version of the BIOS is also important. To make things worse, the problem could be introduced into one BIOS version and fixes in another later version. My feeling is that trying to make guesses here will open a can of worms we better keep closed. The strategy for now has been to search for broken configurations and disable interrupt remapping when they are detected. Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
