Re: [PATCH v11 04/10] iommu/vt-d: functions to copy data from old mem
On Mon, 2015-05-11 at 17:52 +0800, 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.
Li, Zhen-hua:
The functions and logics.
Surely this isn't specific to the Intel IOMMU? Shouldn't it live
elsewhere — either in generic IOMMU code or perhaps in generic kexec
support code?
Don't we need to solve the same kexec problem on *all* platforms with
an IOMMU, and won't they all need something like this?
And I think you're misusing VTD_PAGE_{SHIFT,MASK} when you should be
using the normal PAGE_{SHIFT,MASK}. And shouldn't physical addresses be
phys_addr_t?
--
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com Intel Corporation
smime.p7s
Description: S/MIME cryptographic signature
___
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
Hi David,
On Mon, Jun 08, 2015 at 03:15:35PM +0100, David Woodhouse wrote:
Surely this isn't specific to the Intel IOMMU? Shouldn't it live
elsewhere — either in generic IOMMU code or perhaps in generic kexec
support code?
I put a bigger rework of this on-top of Zhen-Hua's patches, you can find
the result in my x86/vt-d branch. With my patches I also removed this
pointer collecting concept and do the iomap_cache and iounmap calls
before the spin-lock is taken, so this problem is now solved
differently.
And I think you're misusing VTD_PAGE_{SHIFT,MASK} when you should be
using the normal PAGE_{SHIFT,MASK}.
I think VT_PAGE_* is correct, since the VT-d driver also runs on ia64.
There the system page-size is different from the VT-d page-size.
And shouldn't physical addresses be phys_addr_t?
This is changed where appropriate, I hope.
Joerg
___
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 Mon, 2015-06-08 at 17:21 +0200, Joerg Roedel wrote:
Hi David,
On Mon, Jun 08, 2015 at 03:15:35PM +0100, David Woodhouse wrote:
Surely this isn't specific to the Intel IOMMU? Shouldn't it live
elsewhere — either in generic IOMMU code or perhaps in generic kexec
support code?
I put a bigger rework of this on-top of Zhen-Hua's patches, you can find
the result in my x86/vt-d branch. With my patches I also removed this
pointer collecting concept and do the iomap_cache and iounmap calls
before the spin-lock is taken, so this problem is now solved
differently.
And I think you're misusing VTD_PAGE_{SHIFT,MASK} when you should be
using the normal PAGE_{SHIFT,MASK}.
I think VT_PAGE_* is correct, since the VT-d driver also runs on ia64.
There the system page-size is different from the VT-d page-size.
That's the problem. In __iommu_load_from_oldmem we start with a
physical address in 'from', convert to a VT-d PFN in 'pfn':
+ pfn = from VTD_PAGE_SHIFT;
.. and then proceed to pass that pfn to non-VT-d functions like
page_is_ram() and pfn_to_kaddr() which really need their input pfn
values to be in terms of PAGE_SHIFT not VTD_PAGE_SHIFT.
But it looks like you've completely eliminated that now (including the
page_is_ram check). So although it *was* wrong, it doesn't matter now.
And shouldn't physical addresses be phys_addr_t?
This is changed where appropriate, I hope.
OK. In fact once it's purely internal to intel-iommu.c it doesn't
matter as much since we don't put page tables in high memory on 32-bit
machines.
--
David WoodhouseOpen Source Technology Centre
david.woodho...@intel.com Intel Corporation
smime.p7s
Description: S/MIME cryptographic signature
___
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
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 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 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.
Li, Zhen-hua:
The functions and logics.
Takao Indoh:
Check if pfn is ram:
if (page_is_ram(pfn))
Signed-off-by: Li, Zhen-Hua zhen-h...@hp.com
Signed-off-by: Takao Indoh indou.ta...@jp.fujitsu.com
---
drivers/iommu/intel-iommu.c | 102
include/linux/intel-iommu.h | 6 +++
2 files changed, 108 insertions(+)
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 07e6118..0b97c15 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -371,6 +371,17 @@ static struct context_entry
*device_to_existing_context_entry(
struct intel_iommu *iommu,
u8 bus, u8 devfn);
+/*
+ * A structure used to store the address allocated by ioremap();
+ * The we need to call iounmap() to free them out of
spin_lock_irqsave/unlock;
+ */
+struct iommu_remapped_entry {
+ struct list_head list;
+ void __iomem *mem;
+};
+static LIST_HEAD(__iommu_remapped_mem);
+static DEFINE_MUTEX(__iommu_mem_list_lock);
+
/*
* This domain is a statically identity mapping domain.
@@ -4833,3 +4844,94 @@ static struct context_entry
*device_to_existing_context_entry(
return ret;
}
+/*
+ * Copy memory from a physically-addressed area into a virtually-addressed
area
+ */
+int __iommu_load_from_oldmem(void *to, unsigned long from, unsigned long
size)
+{
+ unsigned long pfn; /* Page Frame Number */
+ size_t csize = (size_t)size;/* Num(bytes to copy) */
+ unsigned long offset; /* Lower 12 bits of to */
comment for above variable are unnecessary, they are clear enough.
BTW, use size_t size in function argument is better.
+ void __iomem *virt_mem;
+ struct iommu_remapped_entry *mapped;
+
+ pfn = from VTD_PAGE_SHIFT;
+ offset = from (~VTD_PAGE_MASK);
+
+ if (page_is_ram(pfn)) {
+ memcpy(to, pfn_to_kaddr(pfn) + offset, csize);
+ } else{
+
+ mapped = kzalloc(sizeof(struct iommu_remapped_entry),
+ GFP_KERNEL);
+ if (!mapped)
+ return -ENOMEM;
+
+ virt_mem = ioremap_cache((unsigned long)from, size);
+ if (!virt_mem) {
+ kfree(mapped);
+ return -ENOMEM;
+ }
+ memcpy(to, virt_mem, size);
csize or size? as previous comment use size_t in argument it will be ok here.
+
+ mutex_lock(__iommu_mem_list_lock);
+ mapped-mem = virt_mem;
+ list_add_tail(mapped-list, __iommu_remapped_mem);
+ mutex_unlock(__iommu_mem_list_lock);
+ }
+ return size;
type mismatch?
+}
+
+/*
+ * Copy memory from a virtually-addressed area into a physically-addressed
area
+ */
+int __iommu_save_to_oldmem(unsigned long to, void *from, unsigned long size)
+{
+ unsigned long pfn; /* Page Frame Number */
+ size_t csize = (size_t)size;/* Num(bytes to copy) */
+ unsigned long offset; /* Lower 12 bits of to */
Ditto about data type and comments..
+ void __iomem *virt_mem;
+ struct iommu_remapped_entry *mapped;
+
+ pfn = to VTD_PAGE_SHIFT;
+ offset = to (~VTD_PAGE_MASK);
+
+ if (page_is_ram(pfn)) {
+ memcpy(pfn_to_kaddr(pfn) + offset, from, csize);
+ } else{
+ mapped = kzalloc(sizeof(struct iommu_remapped_entry),
+ GFP_KERNEL);
+ if (!mapped)
+ return -ENOMEM;
+
+ virt_mem = ioremap_cache((unsigned long)to, size);
+ if (!virt_mem) {
+ kfree(mapped);
+ return -ENOMEM;
+ }
+ memcpy(virt_mem, from, size);
+ mutex_lock(__iommu_mem_list_lock);
+ mapped-mem = virt_mem;
+ list_add_tail(mapped-list, __iommu_remapped_mem);
+ mutex_unlock(__iommu_mem_list_lock);
+ }
+ return size;
+}
Above two functions looks duplicate, can they be merged as one function and
add a argument about direction?
+
+/*
+ * Free the mapped memory for ioremap;
+ */
+int __iommu_free_mapped_mem(void)
+{
+ struct iommu_remapped_entry *mem_entry, *tmp;
+
+ mutex_lock(__iommu_mem_list_lock);
+ list_for_each_entry_safe(mem_entry, tmp, __iommu_remapped_mem, list) {
+ iounmap(mem_entry-mem);
+ list_del(mem_entry-list);
+ kfree(mem_entry);
+ }
+
