| Message ID | 1431337974-545-5-git-send-email-zhen-hual@hp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Bjorn Helgaas |
| Headers | show |
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-hual@hp.com> > Signed-off-by: Takao Indoh <indou.takao@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); > + } > + mutex_unlock(&__iommu_mem_list_lock); > + return 0; > +} > + > diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h > index 796ef96..ced1fac 100644 > --- a/include/linux/intel-iommu.h > +++ b/include/linux/intel-iommu.h > @@ -380,4 +380,10 @@ extern int dmar_ir_support(void); > > extern const struct attribute_group *intel_iommu_groups[]; > > +extern int __iommu_load_from_oldmem(void *to, unsigned long from, > + unsigned long size); > +extern int __iommu_save_to_oldmem(unsigned long to, void *from, > + unsigned long size); > +extern int __iommu_free_mapped_mem(void); > + > #endif > -- > 2.0.0-rc0 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 > -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-pci" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 */ + 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); + + 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; +} + +/* + * 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 */ + 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; +} + +/* + * 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); + } + mutex_unlock(&__iommu_mem_list_lock); + return 0; +} + diff --git a/include/linux/intel-iommu.h b/include/linux/intel-iommu.h index 796ef96..ced1fac 100644 --- a/include/linux/intel-iommu.h +++ b/include/linux/intel-iommu.h @@ -380,4 +380,10 @@ extern int dmar_ir_support(void); extern const struct attribute_group *intel_iommu_groups[]; +extern int __iommu_load_from_oldmem(void *to, unsigned long from, + unsigned long size); +extern int __iommu_save_to_oldmem(unsigned long to, void *from, + unsigned long size); +extern int __iommu_free_mapped_mem(void); + #endif
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-hual@hp.com> Signed-off-by: Takao Indoh <indou.takao@jp.fujitsu.com> --- drivers/iommu/intel-iommu.c | 102 ++++++++++++++++++++++++++++++++++++++++++++ include/linux/intel-iommu.h | 6 +++ 2 files changed, 108 insertions(+)