* [Patch v4 10/12] memory-hotplug: memory_hotplug: clear zone when removing the memory
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
When a memory is added, we update zone's and pgdat's start_pfn and
spanned_pages in the function __add_zone(). So we should revert them
when the memory is removed.
The patch adds a new function __remove_zone() to do this.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
mm/memory_hotplug.c | 207 ++++++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 207 insertions(+)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 7797e91..aa97d56 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -301,10 +301,213 @@ static int __meminit __add_section(int nid, struct zone *zone,
return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
}
+/* find the smallest valid pfn in the range [start_pfn, end_pfn) */
+static int find_smallest_section_pfn(int nid, struct zone *zone,
+ unsigned long start_pfn,
+ unsigned long end_pfn)
+{
+ struct mem_section *ms;
+
+ for (; start_pfn < end_pfn; start_pfn += PAGES_PER_SECTION) {
+ ms = __pfn_to_section(start_pfn);
+
+ if (unlikely(!valid_section(ms)))
+ continue;
+
+ if (unlikely(pfn_to_nid(start_pfn) != nid))
+ continue;
+
+ if (zone && zone != page_zone(pfn_to_page(start_pfn)))
+ continue;
+
+ return start_pfn;
+ }
+
+ return 0;
+}
+
+/* find the biggest valid pfn in the range [start_pfn, end_pfn). */
+static int find_biggest_section_pfn(int nid, struct zone *zone,
+ unsigned long start_pfn,
+ unsigned long end_pfn)
+{
+ struct mem_section *ms;
+ unsigned long pfn;
+
+ /* pfn is the end pfn of a memory section. */
+ pfn = end_pfn - 1;
+ for (; pfn >= start_pfn; pfn -= PAGES_PER_SECTION) {
+ ms = __pfn_to_section(pfn);
+
+ if (unlikely(!valid_section(ms)))
+ continue;
+
+ if (unlikely(pfn_to_nid(pfn) != nid))
+ continue;
+
+ if (zone && zone != page_zone(pfn_to_page(pfn)))
+ continue;
+
+ return pfn;
+ }
+
+ return 0;
+}
+
+static void shrink_zone_span(struct zone *zone, unsigned long start_pfn,
+ unsigned long end_pfn)
+{
+ unsigned long zone_start_pfn = zone->zone_start_pfn;
+ unsigned long zone_end_pfn = zone->zone_start_pfn + zone->spanned_pages;
+ unsigned long pfn;
+ struct mem_section *ms;
+ int nid = zone_to_nid(zone);
+
+ zone_span_writelock(zone);
+ if (zone_start_pfn == start_pfn) {
+ /*
+ * If the section is smallest section in the zone, it need
+ * shrink zone->zone_start_pfn and zone->zone_spanned_pages.
+ * In this case, we find second smallest valid mem_section
+ * for shrinking zone.
+ */
+ pfn = find_smallest_section_pfn(nid, zone, end_pfn,
+ zone_end_pfn);
+ if (pfn) {
+ zone->zone_start_pfn = pfn;
+ zone->spanned_pages = zone_end_pfn - pfn;
+ }
+ } else if (zone_end_pfn == end_pfn) {
+ /*
+ * If the section is biggest section in the zone, it need
+ * shrink zone->spanned_pages.
+ * In this case, we find second biggest valid mem_section for
+ * shrinking zone.
+ */
+ pfn = find_biggest_section_pfn(nid, zone, zone_start_pfn,
+ start_pfn);
+ if (pfn)
+ zone->spanned_pages = pfn - zone_start_pfn + 1;
+ }
+
+ /*
+ * The section is not biggest or smallest mem_section in the zone, it
+ * only creates a hole in the zone. So in this case, we need not
+ * change the zone. But perhaps, the zone has only hole data. Thus
+ * it check the zone has only hole or not.
+ */
+ pfn = zone_start_pfn;
+ for (; pfn < zone_end_pfn; pfn += PAGES_PER_SECTION) {
+ ms = __pfn_to_section(pfn);
+
+ if (unlikely(!valid_section(ms)))
+ continue;
+
+ if (page_zone(pfn_to_page(pfn)) != zone)
+ continue;
+
+ /* If the section is current section, it continues the loop */
+ if (start_pfn == pfn)
+ continue;
+
+ /* If we find valid section, we have nothing to do */
+ zone_span_writeunlock(zone);
+ return;
+ }
+
+ /* The zone has no valid section */
+ zone->zone_start_pfn = 0;
+ zone->spanned_pages = 0;
+ zone_span_writeunlock(zone);
+}
+
+static void shrink_pgdat_span(struct pglist_data *pgdat,
+ unsigned long start_pfn, unsigned long end_pfn)
+{
+ unsigned long pgdat_start_pfn = pgdat->node_start_pfn;
+ unsigned long pgdat_end_pfn =
+ pgdat->node_start_pfn + pgdat->node_spanned_pages;
+ unsigned long pfn;
+ struct mem_section *ms;
+ int nid = pgdat->node_id;
+
+ if (pgdat_start_pfn == start_pfn) {
+ /*
+ * If the section is smallest section in the pgdat, it need
+ * shrink pgdat->node_start_pfn and pgdat->node_spanned_pages.
+ * In this case, we find second smallest valid mem_section
+ * for shrinking zone.
+ */
+ pfn = find_smallest_section_pfn(nid, NULL, end_pfn,
+ pgdat_end_pfn);
+ if (pfn) {
+ pgdat->node_start_pfn = pfn;
+ pgdat->node_spanned_pages = pgdat_end_pfn - pfn;
+ }
+ } else if (pgdat_end_pfn == end_pfn) {
+ /*
+ * If the section is biggest section in the pgdat, it need
+ * shrink pgdat->node_spanned_pages.
+ * In this case, we find second biggest valid mem_section for
+ * shrinking zone.
+ */
+ pfn = find_biggest_section_pfn(nid, NULL, pgdat_start_pfn,
+ start_pfn);
+ if (pfn)
+ pgdat->node_spanned_pages = pfn - pgdat_start_pfn + 1;
+ }
+
+ /*
+ * If the section is not biggest or smallest mem_section in the pgdat,
+ * it only creates a hole in the pgdat. So in this case, we need not
+ * change the pgdat.
+ * But perhaps, the pgdat has only hole data. Thus it check the pgdat
+ * has only hole or not.
+ */
+ pfn = pgdat_start_pfn;
+ for (; pfn < pgdat_end_pfn; pfn += PAGES_PER_SECTION) {
+ ms = __pfn_to_section(pfn);
+
+ if (unlikely(!valid_section(ms)))
+ continue;
+
+ if (pfn_to_nid(pfn) != nid)
+ continue;
+
+ /* If the section is current section, it continues the loop */
+ if (start_pfn == pfn)
+ continue;
+
+ /* If we find valid section, we have nothing to do */
+ return;
+ }
+
+ /* The pgdat has no valid section */
+ pgdat->node_start_pfn = 0;
+ pgdat->node_spanned_pages = 0;
+}
+
+static void __remove_zone(struct zone *zone, unsigned long start_pfn)
+{
+ struct pglist_data *pgdat = zone->zone_pgdat;
+ int nr_pages = PAGES_PER_SECTION;
+ int zone_type;
+ unsigned long flags;
+
+ zone_type = zone - pgdat->node_zones;
+
+ pgdat_resize_lock(zone->zone_pgdat, &flags);
+ shrink_zone_span(zone, start_pfn, start_pfn + nr_pages);
+ shrink_pgdat_span(pgdat, start_pfn, start_pfn + nr_pages);
+ pgdat_resize_unlock(zone->zone_pgdat, &flags);
+}
+
static int __remove_section(struct zone *zone, struct mem_section *ms)
{
unsigned long flags;
struct pglist_data *pgdat = zone->zone_pgdat;
+ unsigned long start_pfn;
+ int scn_nr;
int ret = -EINVAL;
if (!valid_section(ms))
@@ -314,6 +517,10 @@ static int __remove_section(struct zone *zone, struct mem_section *ms)
if (ret)
return ret;
+ scn_nr = __section_nr(ms);
+ start_pfn = section_nr_to_pfn(scn_nr);
+ __remove_zone(zone, start_pfn);
+
pgdat_resize_lock(pgdat, &flags);
sparse_remove_one_section(zone, ms);
pgdat_resize_unlock(pgdat, &flags);
--
1.8.0
^ permalink raw reply related
* [Patch v4 09/12] memory-hotplug: remove page table of x86_64 architecture
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Jiang Liu, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu,
paulus, Minchan Kim, KOSAKI Motohiro, David Rientjes,
Christoph Lameter, Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
For hot removing memory, we sholud remove page table about the memory.
So the patch searches a page table about the removed memory, and clear
page table.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
Signed-off-by: Jiang Liu <jiang.liu@huawei.com>
---
arch/x86/include/asm/pgtable_types.h | 1 +
arch/x86/mm/init_64.c | 231 +++++++++++++++++++++++++++++++++++
arch/x86/mm/pageattr.c | 47 +++----
3 files changed, 257 insertions(+), 22 deletions(-)
diff --git a/arch/x86/include/asm/pgtable_types.h b/arch/x86/include/asm/pgtable_types.h
index ec8a1fc..fb0c24d 100644
--- a/arch/x86/include/asm/pgtable_types.h
+++ b/arch/x86/include/asm/pgtable_types.h
@@ -332,6 +332,7 @@ static inline void update_page_count(int level, unsigned long pages) { }
* as a pte too.
*/
extern pte_t *lookup_address(unsigned long address, unsigned int *level);
+extern int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase);
#endif /* !__ASSEMBLY__ */
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index e85626d..23d932a 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -680,6 +680,235 @@ int arch_add_memory(int nid, u64 start, u64 size)
}
EXPORT_SYMBOL_GPL(arch_add_memory);
+static inline void free_pagetable(struct page *page)
+{
+ struct zone *zone;
+ bool bootmem = false;
+
+ /* bootmem page has reserved flag */
+ if (PageReserved(page)) {
+ __ClearPageReserved(page);
+ bootmem = true;
+ }
+
+ __free_page(page);
+
+ if (bootmem) {
+ zone = page_zone(page);
+ zone_span_writelock(zone);
+ zone->present_pages++;
+ zone_span_writeunlock(zone);
+ totalram_pages++;
+ }
+}
+
+static void free_pte_table(pte_t *pte_start, pmd_t *pmd)
+{
+ pte_t *pte;
+ int i;
+
+ for (i = 0; i < PTRS_PER_PTE; i++) {
+ pte = pte_start + i;
+ if (pte_val(*pte))
+ return;
+ }
+
+ /* free a pte talbe */
+ free_pagetable(pmd_page(*pmd));
+ pmd_clear(pmd);
+}
+
+static void free_pmd_table(pmd_t *pmd_start, pud_t *pud)
+{
+ pmd_t *pmd;
+ int i;
+
+ for (i = 0; i < PTRS_PER_PMD; i++) {
+ pmd = pmd_start + i;
+ if (pmd_val(*pmd))
+ return;
+ }
+
+ /* free a pmd talbe */
+ free_pagetable(pud_page(*pud));
+ pud_clear(pud);
+}
+
+/* return true if pgd is changed, otherwise return false */
+static bool free_pud_table(pud_t *pud_start, pgd_t *pgd)
+{
+ pud_t *pud;
+ int i;
+
+ for (i = 0; i < PTRS_PER_PUD; i++) {
+ pud = pud_start + i;
+ if (pud_val(*pud))
+ return false;
+ }
+
+ /* free a pud table */
+ free_pagetable(pgd_page(*pgd));
+ pgd_clear(pgd);
+
+ return true;
+}
+
+static void __meminit
+phys_pte_remove(pte_t *pte_page, unsigned long addr, unsigned long end)
+{
+ unsigned pages = 0;
+ int i = pte_index(addr);
+
+ pte_t *pte = pte_page + pte_index(addr);
+
+ for (; i < PTRS_PER_PTE; i++, addr += PAGE_SIZE, pte++) {
+
+ if (addr >= end)
+ break;
+
+ if (!pte_present(*pte))
+ continue;
+
+ pages++;
+ set_pte(pte, __pte(0));
+ }
+
+ update_page_count(PG_LEVEL_4K, -pages);
+}
+
+static void __meminit
+phys_pmd_remove(pmd_t *pmd_page, unsigned long addr, unsigned long end)
+{
+ unsigned long pages = 0, next;
+ int i = pmd_index(addr);
+
+ for (; i < PTRS_PER_PMD && addr < end; i++, addr = next) {
+ unsigned long pte_phys;
+ pmd_t *pmd = pmd_page + pmd_index(addr);
+ pte_t *pte;
+
+ next = pmd_addr_end(addr, end);
+
+ if (!pmd_present(*pmd))
+ continue;
+
+ if (pmd_large(*pmd)) {
+ if (IS_ALIGNED(addr, PMD_SIZE) &&
+ IS_ALIGNED(next, PMD_SIZE)) {
+ set_pmd(pmd, __pmd(0));
+ pages++;
+ continue;
+ }
+
+ /*
+ * We use 2M page, but we need to remove part of them,
+ * so split 2M page to 4K page.
+ */
+ pte = alloc_low_page(&pte_phys);
+ BUG_ON(!pte);
+ __split_large_page((pte_t *)pmd,
+ (unsigned long)__va(addr), pte);
+
+ spin_lock(&init_mm.page_table_lock);
+ pmd_populate_kernel(&init_mm, pmd, __va(pte_phys));
+ spin_unlock(&init_mm.page_table_lock);
+
+ /* Do a global flush tlb after splitting a large page */
+ flush_tlb_all();
+ }
+
+ spin_lock(&init_mm.page_table_lock);
+ pte = map_low_page((pte_t *)pmd_page_vaddr(*pmd));
+ phys_pte_remove(pte, addr, next);
+ free_pte_table(pte, pmd);
+ unmap_low_page(pte);
+ spin_unlock(&init_mm.page_table_lock);
+ }
+ update_page_count(PG_LEVEL_2M, -pages);
+}
+
+static void __meminit
+phys_pud_remove(pud_t *pud_page, unsigned long addr, unsigned long end)
+{
+ unsigned long pages = 0, next;
+ int i = pud_index(addr);
+
+ for (; i < PTRS_PER_PUD && addr < end; i++, addr = next) {
+ unsigned long pmd_phys;
+ pud_t *pud = pud_page + pud_index(addr);
+ pmd_t *pmd;
+
+ next = pud_addr_end(addr, end);
+
+ if (!pud_present(*pud))
+ continue;
+
+ if (pud_large(*pud)) {
+ if (IS_ALIGNED(addr, PUD_SIZE) &&
+ IS_ALIGNED(next, PUD_SIZE)) {
+ set_pud(pud, __pud(0));
+ pages++;
+ continue;
+ }
+
+ /*
+ * We use 1G page, but we need to remove part of them,
+ * so split 1G page to 2M page.
+ */
+ pmd = alloc_low_page(&pmd_phys);
+ BUG_ON(!pmd);
+ __split_large_page((pte_t *)pud,
+ (unsigned long)__va(addr),
+ (pte_t *)pmd);
+
+ spin_lock(&init_mm.page_table_lock);
+ pud_populate(&init_mm, pud, __va(pmd_phys));
+ spin_unlock(&init_mm.page_table_lock);
+
+ /* Do a global flush tlb after splitting a large page */
+ flush_tlb_all();
+ }
+
+ pmd = map_low_page((pmd_t *)pud_page_vaddr(*pud));
+ phys_pmd_remove(pmd, addr, next);
+ free_pmd_table(pmd, pud);
+ unmap_low_page(pmd);
+ }
+
+ update_page_count(PG_LEVEL_1G, -pages);
+}
+
+void __meminit
+kernel_physical_mapping_remove(unsigned long start, unsigned long end)
+{
+ unsigned long next;
+ bool pgd_changed = false;
+
+ start = (unsigned long)__va(start);
+ end = (unsigned long)__va(end);
+
+ for (; start < end; start = next) {
+ pgd_t *pgd = pgd_offset_k(start);
+ pud_t *pud;
+
+ next = pgd_addr_end(start, end);
+
+ if (!pgd_present(*pgd))
+ continue;
+
+ pud = map_low_page((pud_t *)pgd_page_vaddr(*pgd));
+ phys_pud_remove(pud, __pa(start), __pa(next));
+ if (free_pud_table(pud, pgd))
+ pgd_changed = true;
+ unmap_low_page(pud);
+ }
+
+ if (pgd_changed)
+ sync_global_pgds(start, end - 1);
+
+ flush_tlb_all();
+}
+
#ifdef CONFIG_MEMORY_HOTREMOVE
int __ref arch_remove_memory(u64 start, u64 size)
{
@@ -692,6 +921,8 @@ int __ref arch_remove_memory(u64 start, u64 size)
ret = __remove_pages(zone, start_pfn, nr_pages);
WARN_ON_ONCE(ret);
+ kernel_physical_mapping_remove(start, start + size);
+
return ret;
}
#endif
diff --git a/arch/x86/mm/pageattr.c b/arch/x86/mm/pageattr.c
index a718e0d..7dcb6f9 100644
--- a/arch/x86/mm/pageattr.c
+++ b/arch/x86/mm/pageattr.c
@@ -501,21 +501,13 @@ out_unlock:
return do_split;
}
-static int split_large_page(pte_t *kpte, unsigned long address)
+int __split_large_page(pte_t *kpte, unsigned long address, pte_t *pbase)
{
unsigned long pfn, pfninc = 1;
unsigned int i, level;
- pte_t *pbase, *tmp;
+ pte_t *tmp;
pgprot_t ref_prot;
- struct page *base;
-
- if (!debug_pagealloc)
- spin_unlock(&cpa_lock);
- base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
- if (!debug_pagealloc)
- spin_lock(&cpa_lock);
- if (!base)
- return -ENOMEM;
+ struct page *base = virt_to_page(pbase);
spin_lock(&pgd_lock);
/*
@@ -523,10 +515,11 @@ static int split_large_page(pte_t *kpte, unsigned long address)
* up for us already:
*/
tmp = lookup_address(address, &level);
- if (tmp != kpte)
- goto out_unlock;
+ if (tmp != kpte) {
+ spin_unlock(&pgd_lock);
+ return 1;
+ }
- pbase = (pte_t *)page_address(base);
paravirt_alloc_pte(&init_mm, page_to_pfn(base));
ref_prot = pte_pgprot(pte_clrhuge(*kpte));
/*
@@ -579,17 +572,27 @@ static int split_large_page(pte_t *kpte, unsigned long address)
* going on.
*/
__flush_tlb_all();
+ spin_unlock(&pgd_lock);
- base = NULL;
+ return 0;
+}
-out_unlock:
- /*
- * If we dropped out via the lookup_address check under
- * pgd_lock then stick the page back into the pool:
- */
- if (base)
+static int split_large_page(pte_t *kpte, unsigned long address)
+{
+ pte_t *pbase;
+ struct page *base;
+
+ if (!debug_pagealloc)
+ spin_unlock(&cpa_lock);
+ base = alloc_pages(GFP_KERNEL | __GFP_NOTRACK, 0);
+ if (!debug_pagealloc)
+ spin_lock(&cpa_lock);
+ if (!base)
+ return -ENOMEM;
+
+ pbase = (pte_t *)page_address(base);
+ if (__split_large_page(kpte, address, pbase))
__free_page(base);
- spin_unlock(&pgd_lock);
return 0;
}
--
1.8.0
^ permalink raw reply related
* [Patch v4 03/12] memory-hotplug: remove redundant codes
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
offlining memory blocks and checking whether memory blocks are offlined
are very similar. This patch introduces a new function to remove
redundant codes.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
mm/memory_hotplug.c | 101 ++++++++++++++++++++++++++++------------------------
1 file changed, 55 insertions(+), 46 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b6d1101..6d06488 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1005,20 +1005,14 @@ int offline_pages(unsigned long start_pfn, unsigned long nr_pages)
return __offline_pages(start_pfn, start_pfn + nr_pages, 120 * HZ);
}
-int remove_memory(u64 start, u64 size)
+static int walk_memory_range(unsigned long start_pfn, unsigned long end_pfn,
+ void *arg, int (*func)(struct memory_block *, void *))
{
struct memory_block *mem = NULL;
struct mem_section *section;
- unsigned long start_pfn, end_pfn;
unsigned long pfn, section_nr;
int ret;
- int return_on_error = 0;
- int retry = 0;
-
- start_pfn = PFN_DOWN(start);
- end_pfn = start_pfn + PFN_DOWN(size);
-repeat:
for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
section_nr = pfn_to_section_nr(pfn);
if (!present_section_nr(section_nr))
@@ -1035,22 +1029,61 @@ repeat:
if (!mem)
continue;
- ret = offline_memory_block(mem);
+ ret = func(mem, arg);
if (ret) {
- if (return_on_error) {
- kobject_put(&mem->dev.kobj);
- return ret;
- } else {
- retry = 1;
- }
+ kobject_put(&mem->dev.kobj);
+ return ret;
}
}
if (mem)
kobject_put(&mem->dev.kobj);
- if (retry) {
- return_on_error = 1;
+ return 0;
+}
+
+static int offline_memory_block_cb(struct memory_block *mem, void *arg)
+{
+ int *ret = arg;
+ int error = offline_memory_block(mem);
+
+ if (error != 0 && *ret == 0)
+ *ret = error;
+
+ return 0;
+}
+
+static int is_memblock_offlined_cb(struct memory_block *mem, void *arg)
+{
+ int ret = !is_memblock_offlined(mem);
+
+ if (unlikely(ret))
+ pr_warn("removing memory fails, because memory "
+ "[%#010llx-%#010llx] is onlined\n",
+ PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)),
+ PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1))-1);
+
+ return ret;
+}
+
+int remove_memory(u64 start, u64 size)
+{
+ unsigned long start_pfn, end_pfn;
+ int ret = 0;
+ int retry = 1;
+
+ start_pfn = PFN_DOWN(start);
+ end_pfn = start_pfn + PFN_DOWN(size);
+
+repeat:
+ walk_memory_range(start_pfn, end_pfn, &ret,
+ offline_memory_block_cb);
+ if (ret) {
+ if (!retry)
+ return ret;
+
+ retry = 0;
+ ret = 0;
goto repeat;
}
@@ -1068,37 +1101,13 @@ repeat:
* memory blocks are offlined.
*/
- for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
- section_nr = pfn_to_section_nr(pfn);
- if (!present_section_nr(section_nr))
- continue;
-
- section = __nr_to_section(section_nr);
- /* same memblock? */
- if (mem)
- if ((section_nr >= mem->start_section_nr) &&
- (section_nr <= mem->end_section_nr))
- continue;
-
- mem = find_memory_block_hinted(section, mem);
- if (!mem)
- continue;
-
- ret = is_memblock_offlined(mem);
- if (!ret) {
- pr_warn("removing memory fails, because memory "
- "[%#010llx-%#010llx] is onlined\n",
- PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)),
- PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1)) - 1);
-
- kobject_put(&mem->dev.kobj);
- unlock_memory_hotplug();
- return ret;
- }
+ ret = walk_memory_range(start_pfn, end_pfn, NULL,
+ is_memblock_offlined_cb);
+ if (ret) {
+ unlock_memory_hotplug();
+ return ret;
}
- if (mem)
- kobject_put(&mem->dev.kobj);
unlock_memory_hotplug();
return 0;
--
1.8.0
^ permalink raw reply related
* [Patch v4 04/12] memory-hotplug: remove /sys/firmware/memmap/X sysfs
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
When (hot)adding memory into system, /sys/firmware/memmap/X/{end, start, type}
sysfs files are created. But there is no code to remove these files. The patch
implements the function to remove them.
Note: The code does not free firmware_map_entry which is allocated by bootmem.
So the patch makes memory leak. But I think the memory leak size is
very samll. And it does not affect the system.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
---
drivers/firmware/memmap.c | 98 +++++++++++++++++++++++++++++++++++++++++++-
include/linux/firmware-map.h | 6 +++
mm/memory_hotplug.c | 5 ++-
3 files changed, 106 insertions(+), 3 deletions(-)
diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
index 90723e6..49be12a 100644
--- a/drivers/firmware/memmap.c
+++ b/drivers/firmware/memmap.c
@@ -21,6 +21,7 @@
#include <linux/types.h>
#include <linux/bootmem.h>
#include <linux/slab.h>
+#include <linux/mm.h>
/*
* Data types ------------------------------------------------------------------
@@ -41,6 +42,7 @@ struct firmware_map_entry {
const char *type; /* type of the memory range */
struct list_head list; /* entry for the linked list */
struct kobject kobj; /* kobject for each entry */
+ unsigned int bootmem:1; /* allocated from bootmem */
};
/*
@@ -79,7 +81,26 @@ static const struct sysfs_ops memmap_attr_ops = {
.show = memmap_attr_show,
};
+
+static inline struct firmware_map_entry *
+to_memmap_entry(struct kobject *kobj)
+{
+ return container_of(kobj, struct firmware_map_entry, kobj);
+}
+
+static void release_firmware_map_entry(struct kobject *kobj)
+{
+ struct firmware_map_entry *entry = to_memmap_entry(kobj);
+
+ if (entry->bootmem)
+ /* There is no way to free memory allocated from bootmem */
+ return;
+
+ kfree(entry);
+}
+
static struct kobj_type memmap_ktype = {
+ .release = release_firmware_map_entry,
.sysfs_ops = &memmap_attr_ops,
.default_attrs = def_attrs,
};
@@ -94,6 +115,7 @@ static struct kobj_type memmap_ktype = {
* in firmware initialisation code in one single thread of execution.
*/
static LIST_HEAD(map_entries);
+static DEFINE_SPINLOCK(map_entries_lock);
/**
* firmware_map_add_entry() - Does the real work to add a firmware memmap entry.
@@ -118,11 +140,25 @@ static int firmware_map_add_entry(u64 start, u64 end,
INIT_LIST_HEAD(&entry->list);
kobject_init(&entry->kobj, &memmap_ktype);
+ spin_lock(&map_entries_lock);
list_add_tail(&entry->list, &map_entries);
+ spin_unlock(&map_entries_lock);
return 0;
}
+/**
+ * firmware_map_remove_entry() - Does the real work to remove a firmware
+ * memmap entry.
+ * @entry: removed entry.
+ **/
+static inline void firmware_map_remove_entry(struct firmware_map_entry *entry)
+{
+ spin_lock(&map_entries_lock);
+ list_del(&entry->list);
+ spin_unlock(&map_entries_lock);
+}
+
/*
* Add memmap entry on sysfs
*/
@@ -144,6 +180,35 @@ static int add_sysfs_fw_map_entry(struct firmware_map_entry *entry)
return 0;
}
+/*
+ * Remove memmap entry on sysfs
+ */
+static inline void remove_sysfs_fw_map_entry(struct firmware_map_entry *entry)
+{
+ kobject_put(&entry->kobj);
+}
+
+/*
+ * Search memmap entry
+ */
+
+static struct firmware_map_entry * __meminit
+firmware_map_find_entry(u64 start, u64 end, const char *type)
+{
+ struct firmware_map_entry *entry;
+
+ spin_lock(&map_entries_lock);
+ list_for_each_entry(entry, &map_entries, list)
+ if ((entry->start == start) && (entry->end == end) &&
+ (!strcmp(entry->type, type))) {
+ spin_unlock(&map_entries_lock);
+ return entry;
+ }
+
+ spin_unlock(&map_entries_lock);
+ return NULL;
+}
+
/**
* firmware_map_add_hotplug() - Adds a firmware mapping entry when we do
* memory hotplug.
@@ -193,9 +258,36 @@ int __init firmware_map_add_early(u64 start, u64 end, const char *type)
if (WARN_ON(!entry))
return -ENOMEM;
+ entry->bootmem = 1;
return firmware_map_add_entry(start, end, type, entry);
}
+/**
+ * firmware_map_remove() - remove a firmware mapping entry
+ * @start: Start of the memory range.
+ * @end: End of the memory range.
+ * @type: Type of the memory range.
+ *
+ * removes a firmware mapping entry.
+ *
+ * Returns 0 on success, or -EINVAL if no entry.
+ **/
+int __meminit firmware_map_remove(u64 start, u64 end, const char *type)
+{
+ struct firmware_map_entry *entry;
+
+ entry = firmware_map_find_entry(start, end - 1, type);
+ if (!entry)
+ return -EINVAL;
+
+ firmware_map_remove_entry(entry);
+
+ /* remove the memmap entry */
+ remove_sysfs_fw_map_entry(entry);
+
+ return 0;
+}
+
/*
* Sysfs functions -------------------------------------------------------------
*/
@@ -217,8 +309,10 @@ static ssize_t type_show(struct firmware_map_entry *entry, char *buf)
return snprintf(buf, PAGE_SIZE, "%s\n", entry->type);
}
-#define to_memmap_attr(_attr) container_of(_attr, struct memmap_attribute, attr)
-#define to_memmap_entry(obj) container_of(obj, struct firmware_map_entry, kobj)
+static inline struct memmap_attribute *to_memmap_attr(struct attribute *attr)
+{
+ return container_of(attr, struct memmap_attribute, attr);
+}
static ssize_t memmap_attr_show(struct kobject *kobj,
struct attribute *attr, char *buf)
diff --git a/include/linux/firmware-map.h b/include/linux/firmware-map.h
index 43fe52fc..71d4fa7 100644
--- a/include/linux/firmware-map.h
+++ b/include/linux/firmware-map.h
@@ -25,6 +25,7 @@
int firmware_map_add_early(u64 start, u64 end, const char *type);
int firmware_map_add_hotplug(u64 start, u64 end, const char *type);
+int firmware_map_remove(u64 start, u64 end, const char *type);
#else /* CONFIG_FIRMWARE_MEMMAP */
@@ -38,6 +39,11 @@ static inline int firmware_map_add_hotplug(u64 start, u64 end, const char *type)
return 0;
}
+static inline int firmware_map_remove(u64 start, u64 end, const char *type)
+{
+ return 0;
+}
+
#endif /* CONFIG_FIRMWARE_MEMMAP */
#endif /* _LINUX_FIRMWARE_MAP_H */
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 6d06488..63d5388 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1066,7 +1066,7 @@ static int is_memblock_offlined_cb(struct memory_block *mem, void *arg)
return ret;
}
-int remove_memory(u64 start, u64 size)
+int __ref remove_memory(u64 start, u64 size)
{
unsigned long start_pfn, end_pfn;
int ret = 0;
@@ -1108,6 +1108,9 @@ repeat:
return ret;
}
+ /* remove memmap entry */
+ firmware_map_remove(start, start + size, "System RAM");
+
unlock_memory_hotplug();
return 0;
--
1.8.0
^ permalink raw reply related
* [Patch v4 00/12] memory-hotplug: hot-remove physical memory
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
The patch-set was divided from following thread's patch-set.
https://lkml.org/lkml/2012/9/5/201
The last version of this patchset:
https://lkml.org/lkml/2012/11/1/93
If you want to know the reason, please read following thread.
https://lkml.org/lkml/2012/10/2/83
The patch-set has only the function of kernel core side for physical
memory hot remove. So if you use the patch, please apply following
patches.
- bug fix for memory hot remove
https://lkml.org/lkml/2012/10/31/269
- acpi framework
https://lkml.org/lkml/2012/10/26/175
The patches can free/remove the following things:
- /sys/firmware/memmap/X/{end, start, type} : [PATCH 2/10]
- mem_section and related sysfs files : [PATCH 3-4/10]
- memmap of sparse-vmemmap : [PATCH 5-7/10]
- page table of removed memory : [RFC PATCH 8/10]
- node and related sysfs files : [RFC PATCH 9-10/10]
* [PATCH 2/10] checks whether the memory can be removed or not.
If you find lack of function for physical memory hot-remove, please let me
know.
How to test this patchset?
1. apply this patchset and build the kernel. MEMORY_HOTPLUG, MEMORY_HOTREMOVE,
ACPI_HOTPLUG_MEMORY must be selected.
2. load the module acpi_memhotplug
3. hotplug the memory device(it depends on your hardware)
You will see the memory device under the directory /sys/bus/acpi/devices/.
Its name is PNP0C80:XX.
4. online/offline pages provided by this memory device
You can write online/offline to /sys/devices/system/memory/memoryX/state to
online/offline pages provided by this memory device
5. hotremove the memory device
You can hotremove the memory device by the hardware, or writing 1 to
/sys/bus/acpi/devices/PNP0C80:XX/eject.
Note: if the memory provided by the memory device is used by the kernel, it
can't be offlined. It is not a bug.
Known problems:
1. hotremoving memory device may cause kernel panicked
This bug will be fixed by Liu Jiang's patch:
https://lkml.org/lkml/2012/7/3/1
Changelogs from v3 to v4:
Patch7: remove unused codes.
Patch8: fix nr_pages that is passed to free_map_bootmem()
Changelogs from v2 to v3:
Patch9: call sync_global_pgds() if pgd is changed
Patch10: fix a problem int the patch
Changelogs from v1 to v2:
Patch1: new patch, offline memory twice. 1st iterate: offline every non primary
memory block. 2nd iterate: offline primary (i.e. first added) memory
block.
Patch3: new patch, no logical change, just remove reduntant codes.
Patch9: merge the patch from wujianguo into this patch. flush tlb on all cpu
after the pagetable is changed.
Patch12: new patch, free node_data when a node is offlined
Wen Congyang (6):
memory-hotplug: try to offline the memory twice to avoid dependence
memory-hotplug: remove redundant codes
memory-hotplug: introduce new function arch_remove_memory() for
removing page table depends on architecture
memory-hotplug: remove page table of x86_64 architecture
memory-hotplug: remove sysfs file of node
memory-hotplug: free node_data when a node is offlined
Yasuaki Ishimatsu (6):
memory-hotplug: check whether all memory blocks are offlined or not
when removing memory
memory-hotplug: remove /sys/firmware/memmap/X sysfs
memory-hotplug: unregister memory section on SPARSEMEM_VMEMMAP
memory-hotplug: implement register_page_bootmem_info_section of
sparse-vmemmap
memory-hotplug: remove memmap of sparse-vmemmap
memory-hotplug: memory_hotplug: clear zone when removing the memory
arch/ia64/mm/discontig.c | 14 ++
arch/ia64/mm/init.c | 18 ++
arch/powerpc/mm/init_64.c | 14 ++
arch/powerpc/mm/mem.c | 12 +
arch/s390/mm/init.c | 12 +
arch/s390/mm/vmem.c | 14 ++
arch/sh/mm/init.c | 17 ++
arch/sparc/mm/init_64.c | 14 ++
arch/tile/mm/init.c | 8 +
arch/x86/include/asm/pgtable_types.h | 1 +
arch/x86/mm/init_32.c | 12 +
arch/x86/mm/init_64.c | 417 +++++++++++++++++++++++++++++++++++
arch/x86/mm/pageattr.c | 47 ++--
drivers/acpi/acpi_memhotplug.c | 8 +-
drivers/base/memory.c | 6 +
drivers/firmware/memmap.c | 98 +++++++-
include/linux/firmware-map.h | 6 +
include/linux/memory_hotplug.h | 15 +-
include/linux/mm.h | 5 +-
mm/memory_hotplug.c | 405 ++++++++++++++++++++++++++++++++--
mm/sparse.c | 19 +-
21 files changed, 1098 insertions(+), 64 deletions(-)
--
1.8.0
^ permalink raw reply
* [Patch v4 05/12] memory-hotplug: introduce new function arch_remove_memory() for removing page table depends on architecture
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
For removing memory, we need to remove page table. But it depends
on architecture. So the patch introduce arch_remove_memory() for
removing page table. Now it only calls __remove_pages().
Note: __remove_pages() for some archtecuture is not implemented
(I don't know how to implement it for s390).
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Benjamin Herrenschmidt <benh@kernel.crashing.org>
CC: Paul Mackerras <paulus@samba.org>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
arch/ia64/mm/init.c | 18 ++++++++++++++++++
arch/powerpc/mm/mem.c | 12 ++++++++++++
arch/s390/mm/init.c | 12 ++++++++++++
arch/sh/mm/init.c | 17 +++++++++++++++++
arch/tile/mm/init.c | 8 ++++++++
arch/x86/mm/init_32.c | 12 ++++++++++++
arch/x86/mm/init_64.c | 15 +++++++++++++++
include/linux/memory_hotplug.h | 1 +
mm/memory_hotplug.c | 2 ++
9 files changed, 97 insertions(+)
diff --git a/arch/ia64/mm/init.c b/arch/ia64/mm/init.c
index 082e383..e333822 100644
--- a/arch/ia64/mm/init.c
+++ b/arch/ia64/mm/init.c
@@ -689,6 +689,24 @@ int arch_add_memory(int nid, u64 start, u64 size)
return ret;
}
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int arch_remove_memory(u64 start, u64 size)
+{
+ unsigned long start_pfn = start >> PAGE_SHIFT;
+ unsigned long nr_pages = size >> PAGE_SHIFT;
+ struct zone *zone;
+ int ret;
+
+ zone = page_zone(pfn_to_page(start_pfn));
+ ret = __remove_pages(zone, start_pfn, nr_pages);
+ if (ret)
+ pr_warn("%s: Problem encountered in __remove_pages() as"
+ " ret=%d\n", __func__, ret);
+
+ return ret;
+}
+#endif
#endif
/*
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 0dba506..09c6451 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -133,6 +133,18 @@ int arch_add_memory(int nid, u64 start, u64 size)
return __add_pages(nid, zone, start_pfn, nr_pages);
}
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int arch_remove_memory(u64 start, u64 size)
+{
+ unsigned long start_pfn = start >> PAGE_SHIFT;
+ unsigned long nr_pages = size >> PAGE_SHIFT;
+ struct zone *zone;
+
+ zone = page_zone(pfn_to_page(start_pfn));
+ return __remove_pages(zone, start_pfn, nr_pages);
+}
+#endif
#endif /* CONFIG_MEMORY_HOTPLUG */
/*
diff --git a/arch/s390/mm/init.c b/arch/s390/mm/init.c
index 81e596c..b565190 100644
--- a/arch/s390/mm/init.c
+++ b/arch/s390/mm/init.c
@@ -257,4 +257,16 @@ int arch_add_memory(int nid, u64 start, u64 size)
vmem_remove_mapping(start, size);
return rc;
}
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int arch_remove_memory(u64 start, u64 size)
+{
+ /*
+ * There is no hardware or firmware interface which could trigger a
+ * hot memory remove on s390. So there is nothing that needs to be
+ * implemented.
+ */
+ return -EBUSY;
+}
+#endif
#endif /* CONFIG_MEMORY_HOTPLUG */
diff --git a/arch/sh/mm/init.c b/arch/sh/mm/init.c
index 82cc576..1057940 100644
--- a/arch/sh/mm/init.c
+++ b/arch/sh/mm/init.c
@@ -558,4 +558,21 @@ int memory_add_physaddr_to_nid(u64 addr)
EXPORT_SYMBOL_GPL(memory_add_physaddr_to_nid);
#endif
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int arch_remove_memory(u64 start, u64 size)
+{
+ unsigned long start_pfn = start >> PAGE_SHIFT;
+ unsigned long nr_pages = size >> PAGE_SHIFT;
+ struct zone *zone;
+ int ret;
+
+ zone = page_zone(pfn_to_page(start_pfn));
+ ret = __remove_pages(zone, start_pfn, nr_pages);
+ if (unlikely(ret))
+ pr_warn("%s: Failed, __remove_pages() == %d\n", __func__,
+ ret);
+
+ return ret;
+}
+#endif
#endif /* CONFIG_MEMORY_HOTPLUG */
diff --git a/arch/tile/mm/init.c b/arch/tile/mm/init.c
index ef29d6c..2749515 100644
--- a/arch/tile/mm/init.c
+++ b/arch/tile/mm/init.c
@@ -935,6 +935,14 @@ int remove_memory(u64 start, u64 size)
{
return -EINVAL;
}
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int arch_remove_memory(u64 start, u64 size)
+{
+ /* TODO */
+ return -EBUSY;
+}
+#endif
#endif
struct kmem_cache *pgd_cache;
diff --git a/arch/x86/mm/init_32.c b/arch/x86/mm/init_32.c
index 11a5800..b19eba4 100644
--- a/arch/x86/mm/init_32.c
+++ b/arch/x86/mm/init_32.c
@@ -839,6 +839,18 @@ int arch_add_memory(int nid, u64 start, u64 size)
return __add_pages(nid, zone, start_pfn, nr_pages);
}
+
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int arch_remove_memory(u64 start, u64 size)
+{
+ unsigned long start_pfn = start >> PAGE_SHIFT;
+ unsigned long nr_pages = size >> PAGE_SHIFT;
+ struct zone *zone;
+
+ zone = page_zone(pfn_to_page(start_pfn));
+ return __remove_pages(zone, start_pfn, nr_pages);
+}
+#endif
#endif
/*
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 3baff25..5675335 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -680,6 +680,21 @@ int arch_add_memory(int nid, u64 start, u64 size)
}
EXPORT_SYMBOL_GPL(arch_add_memory);
+#ifdef CONFIG_MEMORY_HOTREMOVE
+int __ref arch_remove_memory(u64 start, u64 size)
+{
+ unsigned long start_pfn = start >> PAGE_SHIFT;
+ unsigned long nr_pages = size >> PAGE_SHIFT;
+ struct zone *zone;
+ int ret;
+
+ zone = page_zone(pfn_to_page(start_pfn));
+ ret = __remove_pages(zone, start_pfn, nr_pages);
+ WARN_ON_ONCE(ret);
+
+ return ret;
+}
+#endif
#endif /* CONFIG_MEMORY_HOTPLUG */
static struct kcore_list kcore_vsyscall;
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 38675e9..191b2d9 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -85,6 +85,7 @@ extern void __online_page_free(struct page *page);
#ifdef CONFIG_MEMORY_HOTREMOVE
extern bool is_pageblock_removable_nolock(struct page *page);
+extern int arch_remove_memory(u64 start, u64 size);
#endif /* CONFIG_MEMORY_HOTREMOVE */
/* reasonably generic interface to expand the physical pages in a zone */
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 63d5388..e741732 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1111,6 +1111,8 @@ repeat:
/* remove memmap entry */
firmware_map_remove(start, start + size, "System RAM");
+ arch_remove_memory(start, size);
+
unlock_memory_hotplug();
return 0;
--
1.8.0
^ permalink raw reply related
* [Patch v4 06/12] memory-hotplug: unregister memory section on SPARSEMEM_VMEMMAP
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Currently __remove_section for SPARSEMEM_VMEMMAP does nothing. But even if
we use SPARSEMEM_VMEMMAP, we can unregister the memory_section.
So the patch add unregister_memory_section() into __remove_section().
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
mm/memory_hotplug.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e741732..171610d 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -279,11 +279,14 @@ static int __meminit __add_section(int nid, struct zone *zone,
#ifdef CONFIG_SPARSEMEM_VMEMMAP
static int __remove_section(struct zone *zone, struct mem_section *ms)
{
- /*
- * XXX: Freeing memmap with vmemmap is not implement yet.
- * This should be removed later.
- */
- return -EBUSY;
+ int ret = -EINVAL;
+
+ if (!valid_section(ms))
+ return ret;
+
+ ret = unregister_memory_section(ms);
+
+ return ret;
}
#else
static int __remove_section(struct zone *zone, struct mem_section *ms)
--
1.8.0
^ permalink raw reply related
* [Patch v4 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
All pages of virtual mapping in removed memory cannot be freed, since some pages
used as PGD/PUD includes not only removed memory but also other memory. So the
patch checks whether page can be freed or not.
How to check whether page can be freed or not?
1. When removing memory, the page structs of the revmoved memory are filled
with 0FD.
2. All page structs are filled with 0xFD on PT/PMD, PT/PMD can be cleared.
In this case, the page used as PT/PMD can be freed.
Applying patch, __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is integrated
into one. So __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is deleted.
Note: vmemmap_kfree() and vmemmap_free_bootmem() are not implemented for ia64,
ppc, s390, and sparc.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
arch/ia64/mm/discontig.c | 8 ++++
arch/powerpc/mm/init_64.c | 8 ++++
arch/s390/mm/vmem.c | 8 ++++
arch/sparc/mm/init_64.c | 8 ++++
arch/x86/mm/init_64.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
include/linux/mm.h | 2 +
mm/memory_hotplug.c | 17 +------
mm/sparse.c | 19 ++++----
8 files changed, 165 insertions(+), 24 deletions(-)
diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
index 33943db..0d23b69 100644
--- a/arch/ia64/mm/discontig.c
+++ b/arch/ia64/mm/discontig.c
@@ -823,6 +823,14 @@ int __meminit vmemmap_populate(struct page *start_page,
return vmemmap_populate_basepages(start_page, size, node);
}
+void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
+{
+}
+
+void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
+{
+}
+
void register_page_bootmem_memmap(unsigned long section_nr,
struct page *start_page, unsigned long size)
{
diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 6466440..df7d155 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -298,6 +298,14 @@ int __meminit vmemmap_populate(struct page *start_page,
return 0;
}
+void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
+{
+}
+
+void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
+{
+}
+
void register_page_bootmem_memmap(unsigned long section_nr,
struct page *start_page, unsigned long size)
{
diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
index 4f4803a..ab69c34 100644
--- a/arch/s390/mm/vmem.c
+++ b/arch/s390/mm/vmem.c
@@ -236,6 +236,14 @@ out:
return ret;
}
+void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
+{
+}
+
+void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
+{
+}
+
void register_page_bootmem_memmap(unsigned long section_nr,
struct page *start_page, unsigned long size)
{
diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index 75a984b..546855d 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -2232,6 +2232,14 @@ void __meminit vmemmap_populate_print_last(void)
}
}
+void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
+{
+}
+
+void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
+{
+}
+
void register_page_bootmem_memmap(unsigned long section_nr,
struct page *start_page, unsigned long size)
{
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 795dae3..e85626d 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -998,6 +998,125 @@ vmemmap_populate(struct page *start_page, unsigned long size, int node)
return 0;
}
+#define PAGE_INUSE 0xFD
+
+unsigned long find_and_clear_pte_page(unsigned long addr, unsigned long end,
+ struct page **pp, int *page_size)
+{
+ pgd_t *pgd;
+ pud_t *pud;
+ pmd_t *pmd;
+ pte_t *pte = NULL;
+ void *page_addr;
+ unsigned long next;
+
+ *pp = NULL;
+
+ pgd = pgd_offset_k(addr);
+ if (pgd_none(*pgd))
+ return pgd_addr_end(addr, end);
+
+ pud = pud_offset(pgd, addr);
+ if (pud_none(*pud))
+ return pud_addr_end(addr, end);
+
+ if (!cpu_has_pse) {
+ next = (addr + PAGE_SIZE) & PAGE_MASK;
+ pmd = pmd_offset(pud, addr);
+ if (pmd_none(*pmd))
+ return next;
+
+ pte = pte_offset_kernel(pmd, addr);
+ if (pte_none(*pte))
+ return next;
+
+ *page_size = PAGE_SIZE;
+ *pp = pte_page(*pte);
+ } else {
+ next = pmd_addr_end(addr, end);
+
+ pmd = pmd_offset(pud, addr);
+ if (pmd_none(*pmd))
+ return next;
+
+ *page_size = PMD_SIZE;
+ *pp = pmd_page(*pmd);
+ }
+
+ /*
+ * Removed page structs are filled with 0xFD.
+ */
+ memset((void *)addr, PAGE_INUSE, next - addr);
+
+ page_addr = page_address(*pp);
+
+ /*
+ * Check the page is filled with 0xFD or not.
+ * memchr_inv() returns the address. In this case, we cannot
+ * clear PTE/PUD entry, since the page is used by other.
+ * So we cannot also free the page.
+ *
+ * memchr_inv() returns NULL. In this case, we can clear
+ * PTE/PUD entry, since the page is not used by other.
+ * So we can also free the page.
+ */
+ if (memchr_inv(page_addr, PAGE_INUSE, *page_size)) {
+ *pp = NULL;
+ return next;
+ }
+
+ if (!cpu_has_pse)
+ pte_clear(&init_mm, addr, pte);
+ else
+ pmd_clear(pmd);
+
+ return next;
+}
+
+void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
+{
+ unsigned long addr = (unsigned long)memmap;
+ unsigned long end = (unsigned long)(memmap + nr_pages);
+ unsigned long next;
+ struct page *page;
+ int page_size;
+
+ for (; addr < end; addr = next) {
+ page = NULL;
+ page_size = 0;
+ next = find_and_clear_pte_page(addr, end, &page, &page_size);
+ if (!page)
+ continue;
+
+ free_pages((unsigned long)page_address(page),
+ get_order(page_size));
+ __flush_tlb_one(addr);
+ }
+}
+
+void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
+{
+ unsigned long addr = (unsigned long)memmap;
+ unsigned long end = (unsigned long)(memmap + nr_pages);
+ unsigned long next;
+ struct page *page;
+ int page_size;
+ unsigned long magic;
+
+ for (; addr < end; addr = next) {
+ page = NULL;
+ page_size = 0;
+ next = find_and_clear_pte_page(addr, end, &page, &page_size);
+ if (!page)
+ continue;
+
+ magic = (unsigned long) page->lru.next;
+ if (magic == SECTION_INFO)
+ put_page_bootmem(page);
+ flush_tlb_kernel_range(addr, end);
+ }
+}
+
void register_page_bootmem_memmap(unsigned long section_nr,
struct page *start_page, unsigned long size)
{
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 5657670..94d5ccd 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1642,6 +1642,8 @@ int vmemmap_populate(struct page *start_page, unsigned long pages, int node);
void vmemmap_populate_print_last(void);
void register_page_bootmem_memmap(unsigned long section_nr, struct page *map,
unsigned long size);
+void vmemmap_kfree(struct page *memmpa, unsigned long nr_pages);
+void vmemmap_free_bootmem(struct page *memmpa, unsigned long nr_pages);
enum mf_flags {
MF_COUNT_INCREASED = 1 << 0,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index ccc11b6..7797e91 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -301,19 +301,6 @@ static int __meminit __add_section(int nid, struct zone *zone,
return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
}
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-static int __remove_section(struct zone *zone, struct mem_section *ms)
-{
- int ret = -EINVAL;
-
- if (!valid_section(ms))
- return ret;
-
- ret = unregister_memory_section(ms);
-
- return ret;
-}
-#else
static int __remove_section(struct zone *zone, struct mem_section *ms)
{
unsigned long flags;
@@ -330,9 +317,9 @@ static int __remove_section(struct zone *zone, struct mem_section *ms)
pgdat_resize_lock(pgdat, &flags);
sparse_remove_one_section(zone, ms);
pgdat_resize_unlock(pgdat, &flags);
- return 0;
+
+ return ret;
}
-#endif
/*
* Reasonably generic function for adding memory. It is
diff --git a/mm/sparse.c b/mm/sparse.c
index fac95f2..c723bc2 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -613,12 +613,13 @@ static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
/* This will make the necessary allocations eventually. */
return sparse_mem_map_populate(pnum, nid);
}
-static void __kfree_section_memmap(struct page *memmap, unsigned long nr_pages)
+static void __kfree_section_memmap(struct page *page, unsigned long nr_pages)
{
- return; /* XXX: Not implemented yet */
+ vmemmap_kfree(page, nr_pages);
}
-static void free_map_bootmem(struct page *page, unsigned long nr_pages)
+static void free_map_bootmem(struct page *page)
{
+ vmemmap_free_bootmem(page, PAGES_PER_SECTION);
}
#else
static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
@@ -658,10 +659,14 @@ static void __kfree_section_memmap(struct page *memmap, unsigned long nr_pages)
get_order(sizeof(struct page) * nr_pages));
}
-static void free_map_bootmem(struct page *page, unsigned long nr_pages)
+static void free_map_bootmem(struct page *page)
{
unsigned long maps_section_nr, removing_section_nr, i;
unsigned long magic;
+ unsigned long nr_pages;
+
+ nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page))
+ >> PAGE_SHIFT;
for (i = 0; i < nr_pages; i++, page++) {
magic = (unsigned long) page->lru.next;
@@ -688,7 +693,6 @@ static void free_map_bootmem(struct page *page, unsigned long nr_pages)
static void free_section_usemap(struct page *memmap, unsigned long *usemap)
{
struct page *usemap_page;
- unsigned long nr_pages;
if (!usemap)
return;
@@ -713,10 +717,7 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap)
struct page *memmap_page;
memmap_page = virt_to_page(memmap);
- nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page))
- >> PAGE_SHIFT;
-
- free_map_bootmem(memmap_page, nr_pages);
+ free_map_bootmem(memmap_page);
}
}
--
1.8.0
^ permalink raw reply related
* [Patch v4 02/12] memory-hotplug: check whether all memory blocks are offlined or not when removing memory
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
We remove the memory like this:
1. lock memory hotplug
2. offline a memory block
3. unlock memory hotplug
4. repeat 1-3 to offline all memory blocks
5. lock memory hotplug
6. remove memory(TODO)
7. unlock memory hotplug
All memory blocks must be offlined before removing memory. But we don't hold
the lock in the whole operation. So we should check whether all memory blocks
are offlined before step6. Otherwise, kernel maybe panicked.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
---
drivers/base/memory.c | 6 ++++++
include/linux/memory_hotplug.h | 1 +
mm/memory_hotplug.c | 47 ++++++++++++++++++++++++++++++++++++++++++
3 files changed, 54 insertions(+)
diff --git a/drivers/base/memory.c b/drivers/base/memory.c
index 86c8821..badb025 100644
--- a/drivers/base/memory.c
+++ b/drivers/base/memory.c
@@ -675,6 +675,12 @@ int offline_memory_block(struct memory_block *mem)
return ret;
}
+/* return true if the memory block is offlined, otherwise, return false */
+bool is_memblock_offlined(struct memory_block *mem)
+{
+ return mem->state == MEM_OFFLINE;
+}
+
/*
* Initialize the sysfs support for memory devices...
*/
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 95573ec..38675e9 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -236,6 +236,7 @@ extern int add_memory(int nid, u64 start, u64 size);
extern int arch_add_memory(int nid, u64 start, u64 size);
extern int offline_pages(unsigned long start_pfn, unsigned long nr_pages);
extern int offline_memory_block(struct memory_block *mem);
+extern bool is_memblock_offlined(struct memory_block *mem);
extern int remove_memory(u64 start, u64 size);
extern int sparse_add_one_section(struct zone *zone, unsigned long start_pfn,
int nr_pages);
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index b825dbc..b6d1101 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1054,6 +1054,53 @@ repeat:
goto repeat;
}
+ lock_memory_hotplug();
+
+ /*
+ * we have offlined all memory blocks like this:
+ * 1. lock memory hotplug
+ * 2. offline a memory block
+ * 3. unlock memory hotplug
+ *
+ * repeat step1-3 to offline the memory block. All memory blocks
+ * must be offlined before removing memory. But we don't hold the
+ * lock in the whole operation. So we should check whether all
+ * memory blocks are offlined.
+ */
+
+ for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
+ section_nr = pfn_to_section_nr(pfn);
+ if (!present_section_nr(section_nr))
+ continue;
+
+ section = __nr_to_section(section_nr);
+ /* same memblock? */
+ if (mem)
+ if ((section_nr >= mem->start_section_nr) &&
+ (section_nr <= mem->end_section_nr))
+ continue;
+
+ mem = find_memory_block_hinted(section, mem);
+ if (!mem)
+ continue;
+
+ ret = is_memblock_offlined(mem);
+ if (!ret) {
+ pr_warn("removing memory fails, because memory "
+ "[%#010llx-%#010llx] is onlined\n",
+ PFN_PHYS(section_nr_to_pfn(mem->start_section_nr)),
+ PFN_PHYS(section_nr_to_pfn(mem->end_section_nr + 1)) - 1);
+
+ kobject_put(&mem->dev.kobj);
+ unlock_memory_hotplug();
+ return ret;
+ }
+ }
+
+ if (mem)
+ kobject_put(&mem->dev.kobj);
+ unlock_memory_hotplug();
+
return 0;
}
#else
--
1.8.0
^ permalink raw reply related
* [Patch v4 07/12] memory-hotplug: implement register_page_bootmem_info_section of sparse-vmemmap
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
For removing memmap region of sparse-vmemmap which is allocated bootmem,
memmap region of sparse-vmemmap needs to be registered by get_page_bootmem().
So the patch searches pages of virtual mapping and registers the pages by
get_page_bootmem().
Note: register_page_bootmem_memmap() is not implemented for ia64, ppc, s390,
and sparc.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
---
arch/ia64/mm/discontig.c | 6 +++++
arch/powerpc/mm/init_64.c | 6 +++++
arch/s390/mm/vmem.c | 6 +++++
arch/sparc/mm/init_64.c | 6 +++++
arch/x86/mm/init_64.c | 52 ++++++++++++++++++++++++++++++++++++++++++
include/linux/memory_hotplug.h | 11 ++-------
include/linux/mm.h | 3 ++-
mm/memory_hotplug.c | 33 +++++++++++++++++++++++----
8 files changed, 109 insertions(+), 14 deletions(-)
diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
index c641333..33943db 100644
--- a/arch/ia64/mm/discontig.c
+++ b/arch/ia64/mm/discontig.c
@@ -822,4 +822,10 @@ int __meminit vmemmap_populate(struct page *start_page,
{
return vmemmap_populate_basepages(start_page, size, node);
}
+
+void register_page_bootmem_memmap(unsigned long section_nr,
+ struct page *start_page, unsigned long size)
+{
+ /* TODO */
+}
#endif
diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
index 95a4529..6466440 100644
--- a/arch/powerpc/mm/init_64.c
+++ b/arch/powerpc/mm/init_64.c
@@ -297,5 +297,11 @@ int __meminit vmemmap_populate(struct page *start_page,
return 0;
}
+
+void register_page_bootmem_memmap(unsigned long section_nr,
+ struct page *start_page, unsigned long size)
+{
+ /* TODO */
+}
#endif /* CONFIG_SPARSEMEM_VMEMMAP */
diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
index 387c7c6..4f4803a 100644
--- a/arch/s390/mm/vmem.c
+++ b/arch/s390/mm/vmem.c
@@ -236,6 +236,12 @@ out:
return ret;
}
+void register_page_bootmem_memmap(unsigned long section_nr,
+ struct page *start_page, unsigned long size)
+{
+ /* TODO */
+}
+
/*
* Add memory segment to the segment list if it doesn't overlap with
* an already present segment.
diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
index 9e28a11..75a984b 100644
--- a/arch/sparc/mm/init_64.c
+++ b/arch/sparc/mm/init_64.c
@@ -2231,6 +2231,12 @@ void __meminit vmemmap_populate_print_last(void)
node_start = 0;
}
}
+
+void register_page_bootmem_memmap(unsigned long section_nr,
+ struct page *start_page, unsigned long size)
+{
+ /* TODO */
+}
#endif /* CONFIG_SPARSEMEM_VMEMMAP */
static void prot_init_common(unsigned long page_none,
diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
index 5675335..795dae3 100644
--- a/arch/x86/mm/init_64.c
+++ b/arch/x86/mm/init_64.c
@@ -998,6 +998,58 @@ vmemmap_populate(struct page *start_page, unsigned long size, int node)
return 0;
}
+void register_page_bootmem_memmap(unsigned long section_nr,
+ struct page *start_page, unsigned long size)
+{
+ unsigned long addr = (unsigned long)start_page;
+ unsigned long end = (unsigned long)(start_page + size);
+ unsigned long next;
+ pgd_t *pgd;
+ pud_t *pud;
+ pmd_t *pmd;
+
+ for (; addr < end; addr = next) {
+ pte_t *pte = NULL;
+
+ pgd = pgd_offset_k(addr);
+ if (pgd_none(*pgd)) {
+ next = (addr + PAGE_SIZE) & PAGE_MASK;
+ continue;
+ }
+ get_page_bootmem(section_nr, pgd_page(*pgd), MIX_SECTION_INFO);
+
+ pud = pud_offset(pgd, addr);
+ if (pud_none(*pud)) {
+ next = (addr + PAGE_SIZE) & PAGE_MASK;
+ continue;
+ }
+ get_page_bootmem(section_nr, pud_page(*pud), MIX_SECTION_INFO);
+
+ if (!cpu_has_pse) {
+ next = (addr + PAGE_SIZE) & PAGE_MASK;
+ pmd = pmd_offset(pud, addr);
+ if (pmd_none(*pmd))
+ continue;
+ get_page_bootmem(section_nr, pmd_page(*pmd),
+ MIX_SECTION_INFO);
+
+ pte = pte_offset_kernel(pmd, addr);
+ if (pte_none(*pte))
+ continue;
+ get_page_bootmem(section_nr, pte_page(*pte),
+ SECTION_INFO);
+ } else {
+ next = pmd_addr_end(addr, end);
+
+ pmd = pmd_offset(pud, addr);
+ if (pmd_none(*pmd))
+ continue;
+ get_page_bootmem(section_nr, pmd_page(*pmd),
+ SECTION_INFO);
+ }
+ }
+}
+
void __meminit vmemmap_populate_print_last(void)
{
if (p_start) {
diff --git a/include/linux/memory_hotplug.h b/include/linux/memory_hotplug.h
index 191b2d9..d4c4402 100644
--- a/include/linux/memory_hotplug.h
+++ b/include/linux/memory_hotplug.h
@@ -163,17 +163,10 @@ static inline void arch_refresh_nodedata(int nid, pg_data_t *pgdat)
#endif /* CONFIG_NUMA */
#endif /* CONFIG_HAVE_ARCH_NODEDATA_EXTENSION */
-#ifdef CONFIG_SPARSEMEM_VMEMMAP
-static inline void register_page_bootmem_info_node(struct pglist_data *pgdat)
-{
-}
-static inline void put_page_bootmem(struct page *page)
-{
-}
-#else
extern void register_page_bootmem_info_node(struct pglist_data *pgdat);
extern void put_page_bootmem(struct page *page);
-#endif
+extern void get_page_bootmem(unsigned long ingo, struct page *page,
+ unsigned long type);
/*
* Lock for memory hotplug guarantees 1) all callbacks for memory hotplug
diff --git a/include/linux/mm.h b/include/linux/mm.h
index bcaab4e..5657670 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -1640,7 +1640,8 @@ int vmemmap_populate_basepages(struct page *start_page,
unsigned long pages, int node);
int vmemmap_populate(struct page *start_page, unsigned long pages, int node);
void vmemmap_populate_print_last(void);
-
+void register_page_bootmem_memmap(unsigned long section_nr, struct page *map,
+ unsigned long size);
enum mf_flags {
MF_COUNT_INCREASED = 1 << 0,
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index 171610d..ccc11b6 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -91,9 +91,8 @@ static void release_memory_resource(struct resource *res)
}
#ifdef CONFIG_MEMORY_HOTPLUG_SPARSE
-#ifndef CONFIG_SPARSEMEM_VMEMMAP
-static void get_page_bootmem(unsigned long info, struct page *page,
- unsigned long type)
+void get_page_bootmem(unsigned long info, struct page *page,
+ unsigned long type)
{
page->lru.next = (struct list_head *) type;
SetPagePrivate(page);
@@ -120,6 +119,7 @@ void __ref put_page_bootmem(struct page *page)
}
+#ifndef CONFIG_SPARSEMEM_VMEMMAP
static void register_page_bootmem_info_section(unsigned long start_pfn)
{
unsigned long *usemap, mapsize, section_nr, i;
@@ -153,6 +153,32 @@ static void register_page_bootmem_info_section(unsigned long start_pfn)
get_page_bootmem(section_nr, page, MIX_SECTION_INFO);
}
+#else
+static void register_page_bootmem_info_section(unsigned long start_pfn)
+{
+ unsigned long *usemap, mapsize, section_nr, i;
+ struct mem_section *ms;
+ struct page *page, *memmap;
+
+ if (!pfn_valid(start_pfn))
+ return;
+
+ section_nr = pfn_to_section_nr(start_pfn);
+ ms = __nr_to_section(section_nr);
+
+ memmap = sparse_decode_mem_map(ms->section_mem_map, section_nr);
+
+ register_page_bootmem_memmap(section_nr, memmap, PAGES_PER_SECTION);
+
+ usemap = __nr_to_section(section_nr)->pageblock_flags;
+ page = virt_to_page(usemap);
+
+ mapsize = PAGE_ALIGN(usemap_size()) >> PAGE_SHIFT;
+
+ for (i = 0; i < mapsize; i++, page++)
+ get_page_bootmem(section_nr, page, MIX_SECTION_INFO);
+}
+#endif
void register_page_bootmem_info_node(struct pglist_data *pgdat)
{
@@ -195,7 +221,6 @@ void register_page_bootmem_info_node(struct pglist_data *pgdat)
register_page_bootmem_info_section(pfn);
}
}
-#endif /* !CONFIG_SPARSEMEM_VMEMMAP */
static void grow_zone_span(struct zone *zone, unsigned long start_pfn,
unsigned long end_pfn)
--
1.8.0
^ permalink raw reply related
* [Patch v4 01/12] memory-hotplug: try to offline the memory twice to avoid dependence
From: Wen Congyang @ 2012-11-27 10:00 UTC (permalink / raw)
To: x86, linux-mm, linux-kernel, linuxppc-dev, linux-acpi, linux-s390,
linux-sh, linux-ia64, cmetcalf, sparclinux
Cc: Len Brown, Wen Congyang, Jianguo Wu, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, Christoph Lameter,
Andrew Morton, Jiang Liu
In-Reply-To: <1354010422-19648-1-git-send-email-wency@cn.fujitsu.com>
memory can't be offlined when CONFIG_MEMCG is selected.
For example: there is a memory device on node 1. The address range
is [1G, 1.5G). You will find 4 new directories memory8, memory9, memory10,
and memory11 under the directory /sys/devices/system/memory/.
If CONFIG_MEMCG is selected, we will allocate memory to store page cgroup
when we online pages. When we online memory8, the memory stored page cgroup
is not provided by this memory device. But when we online memory9, the memory
stored page cgroup may be provided by memory8. So we can't offline memory8
now. We should offline the memory in the reversed order.
When the memory device is hotremoved, we will auto offline memory provided
by this memory device. But we don't know which memory is onlined first, so
offlining memory may fail. In such case, iterate twice to offline the memory.
1st iterate: offline every non primary memory block.
2nd iterate: offline primary (i.e. first added) memory block.
This idea is suggested by KOSAKI Motohiro.
CC: David Rientjes <rientjes@google.com>
CC: Jiang Liu <liuj97@gmail.com>
CC: Len Brown <len.brown@intel.com>
CC: Christoph Lameter <cl@linux.com>
Cc: Minchan Kim <minchan.kim@gmail.com>
CC: Andrew Morton <akpm@linux-foundation.org>
CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
---
mm/memory_hotplug.c | 16 ++++++++++++++--
1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
index e4eeaca..b825dbc 100644
--- a/mm/memory_hotplug.c
+++ b/mm/memory_hotplug.c
@@ -1012,10 +1012,13 @@ int remove_memory(u64 start, u64 size)
unsigned long start_pfn, end_pfn;
unsigned long pfn, section_nr;
int ret;
+ int return_on_error = 0;
+ int retry = 0;
start_pfn = PFN_DOWN(start);
end_pfn = start_pfn + PFN_DOWN(size);
+repeat:
for (pfn = start_pfn; pfn < end_pfn; pfn += PAGES_PER_SECTION) {
section_nr = pfn_to_section_nr(pfn);
if (!present_section_nr(section_nr))
@@ -1034,14 +1037,23 @@ int remove_memory(u64 start, u64 size)
ret = offline_memory_block(mem);
if (ret) {
- kobject_put(&mem->dev.kobj);
- return ret;
+ if (return_on_error) {
+ kobject_put(&mem->dev.kobj);
+ return ret;
+ } else {
+ retry = 1;
+ }
}
}
if (mem)
kobject_put(&mem->dev.kobj);
+ if (retry) {
+ return_on_error = 1;
+ goto repeat;
+ }
+
return 0;
}
#else
--
1.8.0
^ permalink raw reply related
* Re: [PATCH v3 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Jianguo Wu @ 2012-11-27 7:14 UTC (permalink / raw)
To: Wen Congyang
Cc: linux-s390, linux-ia64, Len Brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, linux-mm, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, sparclinux,
Christoph Lameter, linuxppc-dev, Andrew Morton, Jiang Liu
In-Reply-To: <50B4625F.1050307@cn.fujitsu.com>
On 2012/11/27 14:49, Wen Congyang wrote:
> At 11/27/2012 01:47 PM, Jianguo Wu Wrote:
>> On 2012/11/1 17:44, Wen Congyang wrote:
>>
>>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>>
>>> All pages of virtual mapping in removed memory cannot be freed, since some pages
>>> used as PGD/PUD includes not only removed memory but also other memory. So the
>>> patch checks whether page can be freed or not.
>>>
>>> How to check whether page can be freed or not?
>>> 1. When removing memory, the page structs of the revmoved memory are filled
>>> with 0FD.
>>> 2. All page structs are filled with 0xFD on PT/PMD, PT/PMD can be cleared.
>>> In this case, the page used as PT/PMD can be freed.
>>>
>>> Applying patch, __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is integrated
>>> into one. So __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is deleted.
>>>
>>> Note: vmemmap_kfree() and vmemmap_free_bootmem() are not implemented for ia64,
>>> ppc, s390, and sparc.
>>>
>>> CC: David Rientjes <rientjes@google.com>
>>> CC: Jiang Liu <liuj97@gmail.com>
>>> CC: Len Brown <len.brown@intel.com>
>>> CC: Christoph Lameter <cl@linux.com>
>>> Cc: Minchan Kim <minchan.kim@gmail.com>
>>> CC: Andrew Morton <akpm@linux-foundation.org>
>>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>>> CC: Wen Congyang <wency@cn.fujitsu.com>
>>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>> ---
>>> arch/ia64/mm/discontig.c | 8 ++++
>>> arch/powerpc/mm/init_64.c | 8 ++++
>>> arch/s390/mm/vmem.c | 8 ++++
>>> arch/sparc/mm/init_64.c | 8 ++++
>>> arch/x86/mm/init_64.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/mm.h | 2 +
>>> mm/memory_hotplug.c | 17 +------
>>> mm/sparse.c | 5 +-
>>> 8 files changed, 158 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
>>> index 33943db..0d23b69 100644
>>> --- a/arch/ia64/mm/discontig.c
>>> +++ b/arch/ia64/mm/discontig.c
>>> @@ -823,6 +823,14 @@ int __meminit vmemmap_populate(struct page *start_page,
>>> return vmemmap_populate_basepages(start_page, size, node);
>>> }
>>>
>>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> void register_page_bootmem_memmap(unsigned long section_nr,
>>> struct page *start_page, unsigned long size)
>>> {
>>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>>> index 6466440..df7d155 100644
>>> --- a/arch/powerpc/mm/init_64.c
>>> +++ b/arch/powerpc/mm/init_64.c
>>> @@ -298,6 +298,14 @@ int __meminit vmemmap_populate(struct page *start_page,
>>> return 0;
>>> }
>>>
>>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> void register_page_bootmem_memmap(unsigned long section_nr,
>>> struct page *start_page, unsigned long size)
>>> {
>>> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
>>> index 4f4803a..ab69c34 100644
>>> --- a/arch/s390/mm/vmem.c
>>> +++ b/arch/s390/mm/vmem.c
>>> @@ -236,6 +236,14 @@ out:
>>> return ret;
>>> }
>>>
>>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> void register_page_bootmem_memmap(unsigned long section_nr,
>>> struct page *start_page, unsigned long size)
>>> {
>>> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
>>> index 75a984b..546855d 100644
>>> --- a/arch/sparc/mm/init_64.c
>>> +++ b/arch/sparc/mm/init_64.c
>>> @@ -2232,6 +2232,14 @@ void __meminit vmemmap_populate_print_last(void)
>>> }
>>> }
>>>
>>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> +}
>>> +
>>> void register_page_bootmem_memmap(unsigned long section_nr,
>>> struct page *start_page, unsigned long size)
>>> {
>>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>>> index 795dae3..e85626d 100644
>>> --- a/arch/x86/mm/init_64.c
>>> +++ b/arch/x86/mm/init_64.c
>>> @@ -998,6 +998,125 @@ vmemmap_populate(struct page *start_page, unsigned long size, int node)
>>> return 0;
>>> }
>>>
>>> +#define PAGE_INUSE 0xFD
>>> +
>>> +unsigned long find_and_clear_pte_page(unsigned long addr, unsigned long end,
>>> + struct page **pp, int *page_size)
>>> +{
>>> + pgd_t *pgd;
>>> + pud_t *pud;
>>> + pmd_t *pmd;
>>> + pte_t *pte = NULL;
>>> + void *page_addr;
>>> + unsigned long next;
>>> +
>>> + *pp = NULL;
>>> +
>>> + pgd = pgd_offset_k(addr);
>>> + if (pgd_none(*pgd))
>>> + return pgd_addr_end(addr, end);
>>> +
>>> + pud = pud_offset(pgd, addr);
>>> + if (pud_none(*pud))
>>> + return pud_addr_end(addr, end);
>>> +
>>> + if (!cpu_has_pse) {
>>> + next = (addr + PAGE_SIZE) & PAGE_MASK;
>>> + pmd = pmd_offset(pud, addr);
>>> + if (pmd_none(*pmd))
>>> + return next;
>>> +
>>> + pte = pte_offset_kernel(pmd, addr);
>>> + if (pte_none(*pte))
>>> + return next;
>>> +
>>> + *page_size = PAGE_SIZE;
>>> + *pp = pte_page(*pte);
>>> + } else {
>>> + next = pmd_addr_end(addr, end);
>>> +
>>> + pmd = pmd_offset(pud, addr);
>>> + if (pmd_none(*pmd))
>>> + return next;
>>> +
>>> + *page_size = PMD_SIZE;
>>> + *pp = pmd_page(*pmd);
>>> + }
>>> +
>>> + /*
>>> + * Removed page structs are filled with 0xFD.
>>> + */
>>> + memset((void *)addr, PAGE_INUSE, next - addr);
>>> +
>>> + page_addr = page_address(*pp);
>>> +
>>> + /*
>>> + * Check the page is filled with 0xFD or not.
>>> + * memchr_inv() returns the address. In this case, we cannot
>>> + * clear PTE/PUD entry, since the page is used by other.
>>> + * So we cannot also free the page.
>>> + *
>>> + * memchr_inv() returns NULL. In this case, we can clear
>>> + * PTE/PUD entry, since the page is not used by other.
>>> + * So we can also free the page.
>>> + */
>>> + if (memchr_inv(page_addr, PAGE_INUSE, *page_size)) {
>>> + *pp = NULL;
>>> + return next;
>>> + }
>>> +
>>> + if (!cpu_has_pse)
>>> + pte_clear(&init_mm, addr, pte);
>>> + else
>>> + pmd_clear(pmd);
>>> +
>>> + return next;
>>> +}
>>> +
>>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> + unsigned long addr = (unsigned long)memmap;
>>> + unsigned long end = (unsigned long)(memmap + nr_pages);
>>> + unsigned long next;
>>> + struct page *page;
>>> + int page_size;
>>> +
>>> + for (; addr < end; addr = next) {
>>> + page = NULL;
>>> + page_size = 0;
>>> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
>>> + if (!page)
>>> + continue;
>>> +
>>> + free_pages((unsigned long)page_address(page),
>>> + get_order(page_size));
>>> + __flush_tlb_one(addr);
>>> + }
>>> +}
>>> +
>>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>>> +{
>>> + unsigned long addr = (unsigned long)memmap;
>>> + unsigned long end = (unsigned long)(memmap + nr_pages);
>>> + unsigned long next;
>>> + struct page *page;
>>> + int page_size;
>>> + unsigned long magic;
>>> +
>>> + for (; addr < end; addr = next) {
>>> + page = NULL;
>>> + page_size = 0;
>>> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
>>> + if (!page)
>>> + continue;
>>> +
>>> + magic = (unsigned long) page->lru.next;
>>> + if (magic == SECTION_INFO)
>>> + put_page_bootmem(page);
>>> + flush_tlb_kernel_range(addr, end);
>>> + }
>>> +}
>>> +
>>> void register_page_bootmem_memmap(unsigned long section_nr,
>>> struct page *start_page, unsigned long size)
>>> {
>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>> index 8e5a56f..42b8723 100644
>>> --- a/include/linux/mm.h
>>> +++ b/include/linux/mm.h
>>> @@ -1642,6 +1642,8 @@ int vmemmap_populate(struct page *start_page, unsigned long pages, int node);
>>> void vmemmap_populate_print_last(void);
>>> void register_page_bootmem_memmap(unsigned long section_nr, struct page *map,
>>> unsigned long size);
>>> +void vmemmap_kfree(struct page *memmpa, unsigned long nr_pages);
>>> +void vmemmap_free_bootmem(struct page *memmpa, unsigned long nr_pages);
>>>
>>> enum mf_flags {
>>> MF_COUNT_INCREASED = 1 << 0,
>>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>>> index db9806c..03153cf 100644
>>> --- a/mm/memory_hotplug.c
>>> +++ b/mm/memory_hotplug.c
>>> @@ -312,19 +312,6 @@ static int __meminit __add_section(int nid, struct zone *zone,
>>> return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
>>> }
>>>
>>> -#ifdef CONFIG_SPARSEMEM_VMEMMAP
>>> -static int __remove_section(struct zone *zone, struct mem_section *ms)
>>> -{
>>> - int ret = -EINVAL;
>>> -
>>> - if (!valid_section(ms))
>>> - return ret;
>>> -
>>> - ret = unregister_memory_section(ms);
>>> -
>>> - return ret;
>>> -}
>>> -#else
>>> static int __remove_section(struct zone *zone, struct mem_section *ms)
>>> {
>>> unsigned long flags;
>>> @@ -341,9 +328,9 @@ static int __remove_section(struct zone *zone, struct mem_section *ms)
>>> pgdat_resize_lock(pgdat, &flags);
>>> sparse_remove_one_section(zone, ms);
>>> pgdat_resize_unlock(pgdat, &flags);
>>> - return 0;
>>> +
>>> + return ret;
>>> }
>>> -#endif
>>>
>>> /*
>>> * Reasonably generic function for adding memory. It is
>>> diff --git a/mm/sparse.c b/mm/sparse.c
>>> index fac95f2..ab9d755 100644
>>> --- a/mm/sparse.c
>>> +++ b/mm/sparse.c
>>> @@ -613,12 +613,13 @@ static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
>>> /* This will make the necessary allocations eventually. */
>>> return sparse_mem_map_populate(pnum, nid);
>>> }
>>> -static void __kfree_section_memmap(struct page *memmap, unsigned long nr_pages)
>>> +static void __kfree_section_memmap(struct page *page, unsigned long nr_pages)
>>> {
>>> - return; /* XXX: Not implemented yet */
>>> + vmemmap_kfree(page, nr_pages);
>>> }
>>> static void free_map_bootmem(struct page *page, unsigned long nr_pages)
>>> {
>>> + vmemmap_free_bootmem(page, nr_pages);
>>> }
>>
>> Hi Congyang,
>> For vmemmap, nr_pages should be PAGES_PER_SECTION for free_map_bootmem(),
>> which is passed by free_section_usemap(), right?
>> But now, nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page)) >> PAGE_SHIFT.
>>
>> Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
>> ---
>> mm/sparse.c | 4 ++++
>> 1 files changed, 4 insertions(+), 0 deletions(-)
>>
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index fac95f2..31e5282 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -713,8 +713,12 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap)
>> struct page *memmap_page;
>> memmap_page = virt_to_page(memmap);
>>
>> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
>> + nr_pages = PAGES_PER_SECTION;
>> +#else
>> nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page))
>> >> PAGE_SHIFT;
>> +#endif
>
> Hmm, to avoid using ifdef, I think we can pass PAGE_PER_SECTION to free_map_bootmem(),
> and calculate how many pages is used to store struct page.
>
yes, it is ugly to use ifdef. Or we can remove parameter--nr_pages of free_map_bootmem(),
and calculate nr_pages in free_map_bootmem() directly.
> Thanks
> Wen Congyang
>
>>
>> free_map_bootmem(memmap_page, nr_pages);
>> }
>
>
> .
>
^ permalink raw reply
* Re: [PATCH v3 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Wen Congyang @ 2012-11-27 6:49 UTC (permalink / raw)
To: Jianguo Wu
Cc: linux-s390, linux-ia64, Len Brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, linux-mm, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, sparclinux,
Christoph Lameter, linuxppc-dev, Andrew Morton, Jiang Liu
In-Reply-To: <50B45400.20800@huawei.com>
At 11/27/2012 01:47 PM, Jianguo Wu Wrote:
> On 2012/11/1 17:44, Wen Congyang wrote:
>
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> All pages of virtual mapping in removed memory cannot be freed, since some pages
>> used as PGD/PUD includes not only removed memory but also other memory. So the
>> patch checks whether page can be freed or not.
>>
>> How to check whether page can be freed or not?
>> 1. When removing memory, the page structs of the revmoved memory are filled
>> with 0FD.
>> 2. All page structs are filled with 0xFD on PT/PMD, PT/PMD can be cleared.
>> In this case, the page used as PT/PMD can be freed.
>>
>> Applying patch, __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is integrated
>> into one. So __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is deleted.
>>
>> Note: vmemmap_kfree() and vmemmap_free_bootmem() are not implemented for ia64,
>> ppc, s390, and sparc.
>>
>> CC: David Rientjes <rientjes@google.com>
>> CC: Jiang Liu <liuj97@gmail.com>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>> ---
>> arch/ia64/mm/discontig.c | 8 ++++
>> arch/powerpc/mm/init_64.c | 8 ++++
>> arch/s390/mm/vmem.c | 8 ++++
>> arch/sparc/mm/init_64.c | 8 ++++
>> arch/x86/mm/init_64.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mm.h | 2 +
>> mm/memory_hotplug.c | 17 +------
>> mm/sparse.c | 5 +-
>> 8 files changed, 158 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
>> index 33943db..0d23b69 100644
>> --- a/arch/ia64/mm/discontig.c
>> +++ b/arch/ia64/mm/discontig.c
>> @@ -823,6 +823,14 @@ int __meminit vmemmap_populate(struct page *start_page,
>> return vmemmap_populate_basepages(start_page, size, node);
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>> index 6466440..df7d155 100644
>> --- a/arch/powerpc/mm/init_64.c
>> +++ b/arch/powerpc/mm/init_64.c
>> @@ -298,6 +298,14 @@ int __meminit vmemmap_populate(struct page *start_page,
>> return 0;
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
>> index 4f4803a..ab69c34 100644
>> --- a/arch/s390/mm/vmem.c
>> +++ b/arch/s390/mm/vmem.c
>> @@ -236,6 +236,14 @@ out:
>> return ret;
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
>> index 75a984b..546855d 100644
>> --- a/arch/sparc/mm/init_64.c
>> +++ b/arch/sparc/mm/init_64.c
>> @@ -2232,6 +2232,14 @@ void __meminit vmemmap_populate_print_last(void)
>> }
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>> index 795dae3..e85626d 100644
>> --- a/arch/x86/mm/init_64.c
>> +++ b/arch/x86/mm/init_64.c
>> @@ -998,6 +998,125 @@ vmemmap_populate(struct page *start_page, unsigned long size, int node)
>> return 0;
>> }
>>
>> +#define PAGE_INUSE 0xFD
>> +
>> +unsigned long find_and_clear_pte_page(unsigned long addr, unsigned long end,
>> + struct page **pp, int *page_size)
>> +{
>> + pgd_t *pgd;
>> + pud_t *pud;
>> + pmd_t *pmd;
>> + pte_t *pte = NULL;
>> + void *page_addr;
>> + unsigned long next;
>> +
>> + *pp = NULL;
>> +
>> + pgd = pgd_offset_k(addr);
>> + if (pgd_none(*pgd))
>> + return pgd_addr_end(addr, end);
>> +
>> + pud = pud_offset(pgd, addr);
>> + if (pud_none(*pud))
>> + return pud_addr_end(addr, end);
>> +
>> + if (!cpu_has_pse) {
>> + next = (addr + PAGE_SIZE) & PAGE_MASK;
>> + pmd = pmd_offset(pud, addr);
>> + if (pmd_none(*pmd))
>> + return next;
>> +
>> + pte = pte_offset_kernel(pmd, addr);
>> + if (pte_none(*pte))
>> + return next;
>> +
>> + *page_size = PAGE_SIZE;
>> + *pp = pte_page(*pte);
>> + } else {
>> + next = pmd_addr_end(addr, end);
>> +
>> + pmd = pmd_offset(pud, addr);
>> + if (pmd_none(*pmd))
>> + return next;
>> +
>> + *page_size = PMD_SIZE;
>> + *pp = pmd_page(*pmd);
>> + }
>> +
>> + /*
>> + * Removed page structs are filled with 0xFD.
>> + */
>> + memset((void *)addr, PAGE_INUSE, next - addr);
>> +
>> + page_addr = page_address(*pp);
>> +
>> + /*
>> + * Check the page is filled with 0xFD or not.
>> + * memchr_inv() returns the address. In this case, we cannot
>> + * clear PTE/PUD entry, since the page is used by other.
>> + * So we cannot also free the page.
>> + *
>> + * memchr_inv() returns NULL. In this case, we can clear
>> + * PTE/PUD entry, since the page is not used by other.
>> + * So we can also free the page.
>> + */
>> + if (memchr_inv(page_addr, PAGE_INUSE, *page_size)) {
>> + *pp = NULL;
>> + return next;
>> + }
>> +
>> + if (!cpu_has_pse)
>> + pte_clear(&init_mm, addr, pte);
>> + else
>> + pmd_clear(pmd);
>> +
>> + return next;
>> +}
>> +
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> + unsigned long addr = (unsigned long)memmap;
>> + unsigned long end = (unsigned long)(memmap + nr_pages);
>> + unsigned long next;
>> + struct page *page;
>> + int page_size;
>> +
>> + for (; addr < end; addr = next) {
>> + page = NULL;
>> + page_size = 0;
>> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
>> + if (!page)
>> + continue;
>> +
>> + free_pages((unsigned long)page_address(page),
>> + get_order(page_size));
>> + __flush_tlb_one(addr);
>> + }
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> + unsigned long addr = (unsigned long)memmap;
>> + unsigned long end = (unsigned long)(memmap + nr_pages);
>> + unsigned long next;
>> + struct page *page;
>> + int page_size;
>> + unsigned long magic;
>> +
>> + for (; addr < end; addr = next) {
>> + page = NULL;
>> + page_size = 0;
>> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
>> + if (!page)
>> + continue;
>> +
>> + magic = (unsigned long) page->lru.next;
>> + if (magic == SECTION_INFO)
>> + put_page_bootmem(page);
>> + flush_tlb_kernel_range(addr, end);
>> + }
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 8e5a56f..42b8723 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1642,6 +1642,8 @@ int vmemmap_populate(struct page *start_page, unsigned long pages, int node);
>> void vmemmap_populate_print_last(void);
>> void register_page_bootmem_memmap(unsigned long section_nr, struct page *map,
>> unsigned long size);
>> +void vmemmap_kfree(struct page *memmpa, unsigned long nr_pages);
>> +void vmemmap_free_bootmem(struct page *memmpa, unsigned long nr_pages);
>>
>> enum mf_flags {
>> MF_COUNT_INCREASED = 1 << 0,
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index db9806c..03153cf 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -312,19 +312,6 @@ static int __meminit __add_section(int nid, struct zone *zone,
>> return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
>> }
>>
>> -#ifdef CONFIG_SPARSEMEM_VMEMMAP
>> -static int __remove_section(struct zone *zone, struct mem_section *ms)
>> -{
>> - int ret = -EINVAL;
>> -
>> - if (!valid_section(ms))
>> - return ret;
>> -
>> - ret = unregister_memory_section(ms);
>> -
>> - return ret;
>> -}
>> -#else
>> static int __remove_section(struct zone *zone, struct mem_section *ms)
>> {
>> unsigned long flags;
>> @@ -341,9 +328,9 @@ static int __remove_section(struct zone *zone, struct mem_section *ms)
>> pgdat_resize_lock(pgdat, &flags);
>> sparse_remove_one_section(zone, ms);
>> pgdat_resize_unlock(pgdat, &flags);
>> - return 0;
>> +
>> + return ret;
>> }
>> -#endif
>>
>> /*
>> * Reasonably generic function for adding memory. It is
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index fac95f2..ab9d755 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -613,12 +613,13 @@ static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
>> /* This will make the necessary allocations eventually. */
>> return sparse_mem_map_populate(pnum, nid);
>> }
>> -static void __kfree_section_memmap(struct page *memmap, unsigned long nr_pages)
>> +static void __kfree_section_memmap(struct page *page, unsigned long nr_pages)
>> {
>> - return; /* XXX: Not implemented yet */
>> + vmemmap_kfree(page, nr_pages);
>> }
>> static void free_map_bootmem(struct page *page, unsigned long nr_pages)
>> {
>> + vmemmap_free_bootmem(page, nr_pages);
>> }
>
> Hi Congyang,
> For vmemmap, nr_pages should be PAGES_PER_SECTION for free_map_bootmem(),
> which is passed by free_section_usemap(), right?
> But now, nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page)) >> PAGE_SHIFT.
>
> Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
> ---
> mm/sparse.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index fac95f2..31e5282 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -713,8 +713,12 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap)
> struct page *memmap_page;
> memmap_page = virt_to_page(memmap);
>
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
> + nr_pages = PAGES_PER_SECTION;
> +#else
> nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page))
> >> PAGE_SHIFT;
> +#endif
Hmm, to avoid using ifdef, I think we can pass PAGE_PER_SECTION to free_map_bootmem(),
and calculate how many pages is used to store struct page.
Thanks
Wen Congyang
>
> free_map_bootmem(memmap_page, nr_pages);
> }
^ permalink raw reply
* Re: [PATCH v3 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Wen Congyang @ 2012-11-27 6:39 UTC (permalink / raw)
To: Jianguo Wu
Cc: linux-s390, linux-ia64, Len Brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, linux-mm, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, sparclinux,
Christoph Lameter, linuxppc-dev, Andrew Morton, Jiang Liu
In-Reply-To: <50B45400.20800@huawei.com>
At 11/27/2012 01:47 PM, Jianguo Wu Wrote:
> On 2012/11/1 17:44, Wen Congyang wrote:
>
>> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>>
>> All pages of virtual mapping in removed memory cannot be freed, since some pages
>> used as PGD/PUD includes not only removed memory but also other memory. So the
>> patch checks whether page can be freed or not.
>>
>> How to check whether page can be freed or not?
>> 1. When removing memory, the page structs of the revmoved memory are filled
>> with 0FD.
>> 2. All page structs are filled with 0xFD on PT/PMD, PT/PMD can be cleared.
>> In this case, the page used as PT/PMD can be freed.
>>
>> Applying patch, __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is integrated
>> into one. So __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is deleted.
>>
>> Note: vmemmap_kfree() and vmemmap_free_bootmem() are not implemented for ia64,
>> ppc, s390, and sparc.
>>
>> CC: David Rientjes <rientjes@google.com>
>> CC: Jiang Liu <liuj97@gmail.com>
>> CC: Len Brown <len.brown@intel.com>
>> CC: Christoph Lameter <cl@linux.com>
>> Cc: Minchan Kim <minchan.kim@gmail.com>
>> CC: Andrew Morton <akpm@linux-foundation.org>
>> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
>> CC: Wen Congyang <wency@cn.fujitsu.com>
>> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>> ---
>> arch/ia64/mm/discontig.c | 8 ++++
>> arch/powerpc/mm/init_64.c | 8 ++++
>> arch/s390/mm/vmem.c | 8 ++++
>> arch/sparc/mm/init_64.c | 8 ++++
>> arch/x86/mm/init_64.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/mm.h | 2 +
>> mm/memory_hotplug.c | 17 +------
>> mm/sparse.c | 5 +-
>> 8 files changed, 158 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
>> index 33943db..0d23b69 100644
>> --- a/arch/ia64/mm/discontig.c
>> +++ b/arch/ia64/mm/discontig.c
>> @@ -823,6 +823,14 @@ int __meminit vmemmap_populate(struct page *start_page,
>> return vmemmap_populate_basepages(start_page, size, node);
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
>> index 6466440..df7d155 100644
>> --- a/arch/powerpc/mm/init_64.c
>> +++ b/arch/powerpc/mm/init_64.c
>> @@ -298,6 +298,14 @@ int __meminit vmemmap_populate(struct page *start_page,
>> return 0;
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
>> index 4f4803a..ab69c34 100644
>> --- a/arch/s390/mm/vmem.c
>> +++ b/arch/s390/mm/vmem.c
>> @@ -236,6 +236,14 @@ out:
>> return ret;
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
>> index 75a984b..546855d 100644
>> --- a/arch/sparc/mm/init_64.c
>> +++ b/arch/sparc/mm/init_64.c
>> @@ -2232,6 +2232,14 @@ void __meminit vmemmap_populate_print_last(void)
>> }
>> }
>>
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
>> index 795dae3..e85626d 100644
>> --- a/arch/x86/mm/init_64.c
>> +++ b/arch/x86/mm/init_64.c
>> @@ -998,6 +998,125 @@ vmemmap_populate(struct page *start_page, unsigned long size, int node)
>> return 0;
>> }
>>
>> +#define PAGE_INUSE 0xFD
>> +
>> +unsigned long find_and_clear_pte_page(unsigned long addr, unsigned long end,
>> + struct page **pp, int *page_size)
>> +{
>> + pgd_t *pgd;
>> + pud_t *pud;
>> + pmd_t *pmd;
>> + pte_t *pte = NULL;
>> + void *page_addr;
>> + unsigned long next;
>> +
>> + *pp = NULL;
>> +
>> + pgd = pgd_offset_k(addr);
>> + if (pgd_none(*pgd))
>> + return pgd_addr_end(addr, end);
>> +
>> + pud = pud_offset(pgd, addr);
>> + if (pud_none(*pud))
>> + return pud_addr_end(addr, end);
>> +
>> + if (!cpu_has_pse) {
>> + next = (addr + PAGE_SIZE) & PAGE_MASK;
>> + pmd = pmd_offset(pud, addr);
>> + if (pmd_none(*pmd))
>> + return next;
>> +
>> + pte = pte_offset_kernel(pmd, addr);
>> + if (pte_none(*pte))
>> + return next;
>> +
>> + *page_size = PAGE_SIZE;
>> + *pp = pte_page(*pte);
>> + } else {
>> + next = pmd_addr_end(addr, end);
>> +
>> + pmd = pmd_offset(pud, addr);
>> + if (pmd_none(*pmd))
>> + return next;
>> +
>> + *page_size = PMD_SIZE;
>> + *pp = pmd_page(*pmd);
>> + }
>> +
>> + /*
>> + * Removed page structs are filled with 0xFD.
>> + */
>> + memset((void *)addr, PAGE_INUSE, next - addr);
>> +
>> + page_addr = page_address(*pp);
>> +
>> + /*
>> + * Check the page is filled with 0xFD or not.
>> + * memchr_inv() returns the address. In this case, we cannot
>> + * clear PTE/PUD entry, since the page is used by other.
>> + * So we cannot also free the page.
>> + *
>> + * memchr_inv() returns NULL. In this case, we can clear
>> + * PTE/PUD entry, since the page is not used by other.
>> + * So we can also free the page.
>> + */
>> + if (memchr_inv(page_addr, PAGE_INUSE, *page_size)) {
>> + *pp = NULL;
>> + return next;
>> + }
>> +
>> + if (!cpu_has_pse)
>> + pte_clear(&init_mm, addr, pte);
>> + else
>> + pmd_clear(pmd);
>> +
>> + return next;
>> +}
>> +
>> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
>> +{
>> + unsigned long addr = (unsigned long)memmap;
>> + unsigned long end = (unsigned long)(memmap + nr_pages);
>> + unsigned long next;
>> + struct page *page;
>> + int page_size;
>> +
>> + for (; addr < end; addr = next) {
>> + page = NULL;
>> + page_size = 0;
>> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
>> + if (!page)
>> + continue;
>> +
>> + free_pages((unsigned long)page_address(page),
>> + get_order(page_size));
>> + __flush_tlb_one(addr);
>> + }
>> +}
>> +
>> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
>> +{
>> + unsigned long addr = (unsigned long)memmap;
>> + unsigned long end = (unsigned long)(memmap + nr_pages);
>> + unsigned long next;
>> + struct page *page;
>> + int page_size;
>> + unsigned long magic;
>> +
>> + for (; addr < end; addr = next) {
>> + page = NULL;
>> + page_size = 0;
>> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
>> + if (!page)
>> + continue;
>> +
>> + magic = (unsigned long) page->lru.next;
>> + if (magic == SECTION_INFO)
>> + put_page_bootmem(page);
>> + flush_tlb_kernel_range(addr, end);
>> + }
>> +}
>> +
>> void register_page_bootmem_memmap(unsigned long section_nr,
>> struct page *start_page, unsigned long size)
>> {
>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>> index 8e5a56f..42b8723 100644
>> --- a/include/linux/mm.h
>> +++ b/include/linux/mm.h
>> @@ -1642,6 +1642,8 @@ int vmemmap_populate(struct page *start_page, unsigned long pages, int node);
>> void vmemmap_populate_print_last(void);
>> void register_page_bootmem_memmap(unsigned long section_nr, struct page *map,
>> unsigned long size);
>> +void vmemmap_kfree(struct page *memmpa, unsigned long nr_pages);
>> +void vmemmap_free_bootmem(struct page *memmpa, unsigned long nr_pages);
>>
>> enum mf_flags {
>> MF_COUNT_INCREASED = 1 << 0,
>> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
>> index db9806c..03153cf 100644
>> --- a/mm/memory_hotplug.c
>> +++ b/mm/memory_hotplug.c
>> @@ -312,19 +312,6 @@ static int __meminit __add_section(int nid, struct zone *zone,
>> return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
>> }
>>
>> -#ifdef CONFIG_SPARSEMEM_VMEMMAP
>> -static int __remove_section(struct zone *zone, struct mem_section *ms)
>> -{
>> - int ret = -EINVAL;
>> -
>> - if (!valid_section(ms))
>> - return ret;
>> -
>> - ret = unregister_memory_section(ms);
>> -
>> - return ret;
>> -}
>> -#else
>> static int __remove_section(struct zone *zone, struct mem_section *ms)
>> {
>> unsigned long flags;
>> @@ -341,9 +328,9 @@ static int __remove_section(struct zone *zone, struct mem_section *ms)
>> pgdat_resize_lock(pgdat, &flags);
>> sparse_remove_one_section(zone, ms);
>> pgdat_resize_unlock(pgdat, &flags);
>> - return 0;
>> +
>> + return ret;
>> }
>> -#endif
>>
>> /*
>> * Reasonably generic function for adding memory. It is
>> diff --git a/mm/sparse.c b/mm/sparse.c
>> index fac95f2..ab9d755 100644
>> --- a/mm/sparse.c
>> +++ b/mm/sparse.c
>> @@ -613,12 +613,13 @@ static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
>> /* This will make the necessary allocations eventually. */
>> return sparse_mem_map_populate(pnum, nid);
>> }
>> -static void __kfree_section_memmap(struct page *memmap, unsigned long nr_pages)
>> +static void __kfree_section_memmap(struct page *page, unsigned long nr_pages)
>> {
>> - return; /* XXX: Not implemented yet */
>> + vmemmap_kfree(page, nr_pages);
>> }
>> static void free_map_bootmem(struct page *page, unsigned long nr_pages)
>> {
>> + vmemmap_free_bootmem(page, nr_pages);
>> }
>
> Hi Congyang,
> For vmemmap, nr_pages should be PAGES_PER_SECTION for free_map_bootmem(),
> which is passed by free_section_usemap(), right?
Yes, you are right. I will merge it into my patchset.
I will sent the new patchset later today.
Thanks
Wen Congyang
> But now, nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page)) >> PAGE_SHIFT.
>
> Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
> ---
> mm/sparse.c | 4 ++++
> 1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/mm/sparse.c b/mm/sparse.c
> index fac95f2..31e5282 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -713,8 +713,12 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap)
> struct page *memmap_page;
> memmap_page = virt_to_page(memmap);
>
> +#ifdef CONFIG_SPARSEMEM_VMEMMAP
> + nr_pages = PAGES_PER_SECTION;
> +#else
> nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page))
> >> PAGE_SHIFT;
> +#endif
>
> free_map_bootmem(memmap_page, nr_pages);
> }
^ permalink raw reply
* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
From: Len Brown @ 2012-11-27 6:15 UTC (permalink / raw)
To: Rafael J. Wysocki
Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
linux-pm, Daniel Lezcano, linux-kernel, linux-acpi,
Srivatsa S. Bhat, Julius Werner, Andrew Morton, linuxppc-dev,
Sameer Nanda
In-Reply-To: <3364211.o5ZCq4Y6Je@vostro.rjw.lan>
>> drivers/idle/intel_idle.c | 14 +-----
Acked-by: Len Brown <len.brown@intel.com>
thanks!
-Len
^ permalink raw reply
* Re: [PATCH] cpuidle: Measure idle state durations with monotonic clock
From: Len Brown @ 2012-11-27 6:14 UTC (permalink / raw)
To: Preeti Murthy
Cc: Kevin Hilman, Deepthi Dharwar, Trinabh Gupta, Lists Linaro-dev,
Peter Zijlstra, linux-pm, Daniel Lezcano, linux-kernel,
Rafael J. Wysocki, linux-acpi, Srivatsa S. Bhat, Julius Werner,
Andrew Morton, linuxppc-dev, Sameer Nanda
In-Reply-To: <CAM4v1pPhaP=pVnFMXUU_nmrOxhY5yPNtbBdQsT5MH37MndK6LQ@mail.gmail.com>
On 11/15/2012 04:04 AM, Preeti Murthy wrote:
> Hi all,
>
> The code looks correct and inviting to me as it has led to good cleanups.
> I dont think passing 0 as the argument to the function
> sched_clock_idle_wakeup_event()
> should lead to problems,as it does not do anything useful with the
> passed arguments.
>
> My only curiosity is what was the purpose of passing idle residency time to
> sched_clock_idle_wakeup_event() when this data could always be retrieved from
> dev->last_residency for each cpu,which gets almost immediately updated.
sched_clock_idle_wakeup_event() is part of the scheduler.
The scheduler doesn't know what a cpuidle_device is, and
probably should not grow such a dependency.
cheers,
-Len Brown, Intel Open Source Technology Center
> But this does not seem to come in way of this patch for now.Anyway I
> have added Peter to
> the list so that he can opine about this issue if possible and needed.
>
> Reviewed-by: Preeti U Murthy <preeti@linux.vnet.ibm.com>
>
>
> Regards
> Preeti U Murthy
>
^ permalink raw reply
* Re: [PATCH v3 08/12] memory-hotplug: remove memmap of sparse-vmemmap
From: Jianguo Wu @ 2012-11-27 5:47 UTC (permalink / raw)
To: Wen Congyang
Cc: linux-s390, linux-ia64, Len Brown, linux-acpi, linux-sh, x86,
linux-kernel, cmetcalf, linux-mm, Yasuaki Ishimatsu, paulus,
Minchan Kim, KOSAKI Motohiro, David Rientjes, sparclinux,
Christoph Lameter, linuxppc-dev, Andrew Morton, Jiang Liu
In-Reply-To: <1351763083-7905-9-git-send-email-wency@cn.fujitsu.com>
On 2012/11/1 17:44, Wen Congyang wrote:
> From: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
>
> All pages of virtual mapping in removed memory cannot be freed, since some pages
> used as PGD/PUD includes not only removed memory but also other memory. So the
> patch checks whether page can be freed or not.
>
> How to check whether page can be freed or not?
> 1. When removing memory, the page structs of the revmoved memory are filled
> with 0FD.
> 2. All page structs are filled with 0xFD on PT/PMD, PT/PMD can be cleared.
> In this case, the page used as PT/PMD can be freed.
>
> Applying patch, __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is integrated
> into one. So __remove_section() of CONFIG_SPARSEMEM_VMEMMAP is deleted.
>
> Note: vmemmap_kfree() and vmemmap_free_bootmem() are not implemented for ia64,
> ppc, s390, and sparc.
>
> CC: David Rientjes <rientjes@google.com>
> CC: Jiang Liu <liuj97@gmail.com>
> CC: Len Brown <len.brown@intel.com>
> CC: Christoph Lameter <cl@linux.com>
> Cc: Minchan Kim <minchan.kim@gmail.com>
> CC: Andrew Morton <akpm@linux-foundation.org>
> CC: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: Wen Congyang <wency@cn.fujitsu.com>
> Signed-off-by: Yasuaki Ishimatsu <isimatu.yasuaki@jp.fujitsu.com>
> ---
> arch/ia64/mm/discontig.c | 8 ++++
> arch/powerpc/mm/init_64.c | 8 ++++
> arch/s390/mm/vmem.c | 8 ++++
> arch/sparc/mm/init_64.c | 8 ++++
> arch/x86/mm/init_64.c | 119 ++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mm.h | 2 +
> mm/memory_hotplug.c | 17 +------
> mm/sparse.c | 5 +-
> 8 files changed, 158 insertions(+), 17 deletions(-)
>
> diff --git a/arch/ia64/mm/discontig.c b/arch/ia64/mm/discontig.c
> index 33943db..0d23b69 100644
> --- a/arch/ia64/mm/discontig.c
> +++ b/arch/ia64/mm/discontig.c
> @@ -823,6 +823,14 @@ int __meminit vmemmap_populate(struct page *start_page,
> return vmemmap_populate_basepages(start_page, size, node);
> }
>
> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> void register_page_bootmem_memmap(unsigned long section_nr,
> struct page *start_page, unsigned long size)
> {
> diff --git a/arch/powerpc/mm/init_64.c b/arch/powerpc/mm/init_64.c
> index 6466440..df7d155 100644
> --- a/arch/powerpc/mm/init_64.c
> +++ b/arch/powerpc/mm/init_64.c
> @@ -298,6 +298,14 @@ int __meminit vmemmap_populate(struct page *start_page,
> return 0;
> }
>
> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> void register_page_bootmem_memmap(unsigned long section_nr,
> struct page *start_page, unsigned long size)
> {
> diff --git a/arch/s390/mm/vmem.c b/arch/s390/mm/vmem.c
> index 4f4803a..ab69c34 100644
> --- a/arch/s390/mm/vmem.c
> +++ b/arch/s390/mm/vmem.c
> @@ -236,6 +236,14 @@ out:
> return ret;
> }
>
> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> void register_page_bootmem_memmap(unsigned long section_nr,
> struct page *start_page, unsigned long size)
> {
> diff --git a/arch/sparc/mm/init_64.c b/arch/sparc/mm/init_64.c
> index 75a984b..546855d 100644
> --- a/arch/sparc/mm/init_64.c
> +++ b/arch/sparc/mm/init_64.c
> @@ -2232,6 +2232,14 @@ void __meminit vmemmap_populate_print_last(void)
> }
> }
>
> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
> +{
> +}
> +
> void register_page_bootmem_memmap(unsigned long section_nr,
> struct page *start_page, unsigned long size)
> {
> diff --git a/arch/x86/mm/init_64.c b/arch/x86/mm/init_64.c
> index 795dae3..e85626d 100644
> --- a/arch/x86/mm/init_64.c
> +++ b/arch/x86/mm/init_64.c
> @@ -998,6 +998,125 @@ vmemmap_populate(struct page *start_page, unsigned long size, int node)
> return 0;
> }
>
> +#define PAGE_INUSE 0xFD
> +
> +unsigned long find_and_clear_pte_page(unsigned long addr, unsigned long end,
> + struct page **pp, int *page_size)
> +{
> + pgd_t *pgd;
> + pud_t *pud;
> + pmd_t *pmd;
> + pte_t *pte = NULL;
> + void *page_addr;
> + unsigned long next;
> +
> + *pp = NULL;
> +
> + pgd = pgd_offset_k(addr);
> + if (pgd_none(*pgd))
> + return pgd_addr_end(addr, end);
> +
> + pud = pud_offset(pgd, addr);
> + if (pud_none(*pud))
> + return pud_addr_end(addr, end);
> +
> + if (!cpu_has_pse) {
> + next = (addr + PAGE_SIZE) & PAGE_MASK;
> + pmd = pmd_offset(pud, addr);
> + if (pmd_none(*pmd))
> + return next;
> +
> + pte = pte_offset_kernel(pmd, addr);
> + if (pte_none(*pte))
> + return next;
> +
> + *page_size = PAGE_SIZE;
> + *pp = pte_page(*pte);
> + } else {
> + next = pmd_addr_end(addr, end);
> +
> + pmd = pmd_offset(pud, addr);
> + if (pmd_none(*pmd))
> + return next;
> +
> + *page_size = PMD_SIZE;
> + *pp = pmd_page(*pmd);
> + }
> +
> + /*
> + * Removed page structs are filled with 0xFD.
> + */
> + memset((void *)addr, PAGE_INUSE, next - addr);
> +
> + page_addr = page_address(*pp);
> +
> + /*
> + * Check the page is filled with 0xFD or not.
> + * memchr_inv() returns the address. In this case, we cannot
> + * clear PTE/PUD entry, since the page is used by other.
> + * So we cannot also free the page.
> + *
> + * memchr_inv() returns NULL. In this case, we can clear
> + * PTE/PUD entry, since the page is not used by other.
> + * So we can also free the page.
> + */
> + if (memchr_inv(page_addr, PAGE_INUSE, *page_size)) {
> + *pp = NULL;
> + return next;
> + }
> +
> + if (!cpu_has_pse)
> + pte_clear(&init_mm, addr, pte);
> + else
> + pmd_clear(pmd);
> +
> + return next;
> +}
> +
> +void vmemmap_kfree(struct page *memmap, unsigned long nr_pages)
> +{
> + unsigned long addr = (unsigned long)memmap;
> + unsigned long end = (unsigned long)(memmap + nr_pages);
> + unsigned long next;
> + struct page *page;
> + int page_size;
> +
> + for (; addr < end; addr = next) {
> + page = NULL;
> + page_size = 0;
> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
> + if (!page)
> + continue;
> +
> + free_pages((unsigned long)page_address(page),
> + get_order(page_size));
> + __flush_tlb_one(addr);
> + }
> +}
> +
> +void vmemmap_free_bootmem(struct page *memmap, unsigned long nr_pages)
> +{
> + unsigned long addr = (unsigned long)memmap;
> + unsigned long end = (unsigned long)(memmap + nr_pages);
> + unsigned long next;
> + struct page *page;
> + int page_size;
> + unsigned long magic;
> +
> + for (; addr < end; addr = next) {
> + page = NULL;
> + page_size = 0;
> + next = find_and_clear_pte_page(addr, end, &page, &page_size);
> + if (!page)
> + continue;
> +
> + magic = (unsigned long) page->lru.next;
> + if (magic == SECTION_INFO)
> + put_page_bootmem(page);
> + flush_tlb_kernel_range(addr, end);
> + }
> +}
> +
> void register_page_bootmem_memmap(unsigned long section_nr,
> struct page *start_page, unsigned long size)
> {
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8e5a56f..42b8723 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1642,6 +1642,8 @@ int vmemmap_populate(struct page *start_page, unsigned long pages, int node);
> void vmemmap_populate_print_last(void);
> void register_page_bootmem_memmap(unsigned long section_nr, struct page *map,
> unsigned long size);
> +void vmemmap_kfree(struct page *memmpa, unsigned long nr_pages);
> +void vmemmap_free_bootmem(struct page *memmpa, unsigned long nr_pages);
>
> enum mf_flags {
> MF_COUNT_INCREASED = 1 << 0,
> diff --git a/mm/memory_hotplug.c b/mm/memory_hotplug.c
> index db9806c..03153cf 100644
> --- a/mm/memory_hotplug.c
> +++ b/mm/memory_hotplug.c
> @@ -312,19 +312,6 @@ static int __meminit __add_section(int nid, struct zone *zone,
> return register_new_memory(nid, __pfn_to_section(phys_start_pfn));
> }
>
> -#ifdef CONFIG_SPARSEMEM_VMEMMAP
> -static int __remove_section(struct zone *zone, struct mem_section *ms)
> -{
> - int ret = -EINVAL;
> -
> - if (!valid_section(ms))
> - return ret;
> -
> - ret = unregister_memory_section(ms);
> -
> - return ret;
> -}
> -#else
> static int __remove_section(struct zone *zone, struct mem_section *ms)
> {
> unsigned long flags;
> @@ -341,9 +328,9 @@ static int __remove_section(struct zone *zone, struct mem_section *ms)
> pgdat_resize_lock(pgdat, &flags);
> sparse_remove_one_section(zone, ms);
> pgdat_resize_unlock(pgdat, &flags);
> - return 0;
> +
> + return ret;
> }
> -#endif
>
> /*
> * Reasonably generic function for adding memory. It is
> diff --git a/mm/sparse.c b/mm/sparse.c
> index fac95f2..ab9d755 100644
> --- a/mm/sparse.c
> +++ b/mm/sparse.c
> @@ -613,12 +613,13 @@ static inline struct page *kmalloc_section_memmap(unsigned long pnum, int nid,
> /* This will make the necessary allocations eventually. */
> return sparse_mem_map_populate(pnum, nid);
> }
> -static void __kfree_section_memmap(struct page *memmap, unsigned long nr_pages)
> +static void __kfree_section_memmap(struct page *page, unsigned long nr_pages)
> {
> - return; /* XXX: Not implemented yet */
> + vmemmap_kfree(page, nr_pages);
> }
> static void free_map_bootmem(struct page *page, unsigned long nr_pages)
> {
> + vmemmap_free_bootmem(page, nr_pages);
> }
Hi Congyang,
For vmemmap, nr_pages should be PAGES_PER_SECTION for free_map_bootmem(),
which is passed by free_section_usemap(), right?
But now, nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page)) >> PAGE_SHIFT.
Signed-off-by: Jianguo Wu <wujianguo@huawei.com>
---
mm/sparse.c | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)
diff --git a/mm/sparse.c b/mm/sparse.c
index fac95f2..31e5282 100644
--- a/mm/sparse.c
+++ b/mm/sparse.c
@@ -713,8 +713,12 @@ static void free_section_usemap(struct page *memmap, unsigned long *usemap)
struct page *memmap_page;
memmap_page = virt_to_page(memmap);
+#ifdef CONFIG_SPARSEMEM_VMEMMAP
+ nr_pages = PAGES_PER_SECTION;
+#else
nr_pages = PAGE_ALIGN(PAGES_PER_SECTION * sizeof(struct page))
>> PAGE_SHIFT;
+#endif
free_map_bootmem(memmap_page, nr_pages);
}
--
1.7.6.1
> #else
> static struct page *__kmalloc_section_memmap(unsigned long nr_pages)
^ permalink raw reply related
* Re: [PATCH 1/3] powerpc: Relocate prom_init.c on 64bit
From: Benjamin Herrenschmidt @ 2012-11-27 5:22 UTC (permalink / raw)
To: Anton Blanchard; +Cc: linuxppc-dev, paulus, amodra
In-Reply-To: <20121127143903.4b5cab17@kryten>
On Tue, 2012-11-27 at 14:39 +1100, Anton Blanchard wrote:
> The ppc64 kernel can get loaded at any address which means
> our very early init code in prom_init.c must be relocatable. We do
> this with a pretty nasty RELOC() macro that we wrap accesses of
> variables with. It is very fragile and sometimes we forget to add a
> RELOC() to an uncommon path or sometimes a compiler change breaks it.
>
> 32bit has a much more elegant solution where we build prom_init.c
> with -mrelocatable and then process the relocations manually.
> Unfortunately we can't do the equivalent on 64bit and we would
> have to build the entire kernel relocatable (-pie), resulting in a
> large increase in kernel footprint (megabytes of relocation data).
> The relocation data will be marked __initdata but it still creates
> more pressure on our already tight memory layout at boot.
>
> Alan Modra pointed out that the 64bit ABI is relocatable even
> if we don't build with -pie, we just need to relocate the TOC.
> This patch implements that idea and relocates the TOC entries of
> prom_init.c. An added bonus is there are very few relocations to
> process which helps keep boot times on simulators down.
>
> gcc does not put 64bit integer constants into the TOC but to be
> safe we may want a build time script which passes through the
> prom_init.c TOC entries to make sure everything looks reasonable.
My only potential objection was that it might have been cleaner to
actually build prom_init.c as a separate binary alltogether and piggy
back it...
Ben.
> Signed-off-by: Anton Blanchard <anton@samba.org>
> ---
>
> To keep the patch small and reviewable, I separated the removal
> of the RELOC macro into a follow up patch.
>
> For simplicity I do the relocation in C but if self brain surgery
> keeps people up at night we can move it into assembly.
>
> Index: b/arch/powerpc/kernel/prom_init.c
> ===================================================================
> --- a/arch/powerpc/kernel/prom_init.c
> +++ b/arch/powerpc/kernel/prom_init.c
> @@ -66,8 +66,8 @@
> * is running at whatever address it has been loaded at.
> * On ppc32 we compile with -mrelocatable, which means that references
> * to extern and static variables get relocated automatically.
> - * On ppc64 we have to relocate the references explicitly with
> - * RELOC. (Note that strings count as static variables.)
> + * ppc64 objects are always relocatable, we just need to relocate the
> + * TOC.
> *
> * Because OF may have mapped I/O devices into the area starting at
> * KERNELBASE, particularly on CHRP machines, we can't safely call
> @@ -79,13 +79,12 @@
> * On ppc64, 64 bit values are truncated to 32 bits (and
> * fortunately don't get interpreted as two arguments).
> */
> +#define RELOC(x) (x)
> +#define ADDR(x) (u32)(unsigned long)(x)
> +
> #ifdef CONFIG_PPC64
> -#define RELOC(x) (*PTRRELOC(&(x)))
> -#define ADDR(x) (u32) add_reloc_offset((unsigned long)(x))
> #define OF_WORKAROUNDS 0
> #else
> -#define RELOC(x) (x)
> -#define ADDR(x) (u32) (x)
> #define OF_WORKAROUNDS of_workarounds
> int of_workarounds;
> #endif
> @@ -334,9 +333,6 @@ static void __init prom_printf(const cha
> struct prom_t *_prom = &RELOC(prom);
>
> va_start(args, format);
> -#ifdef CONFIG_PPC64
> - format = PTRRELOC(format);
> -#endif
> for (p = format; *p != 0; p = q) {
> for (q = p; *q != 0 && *q != '\n' && *q != '%'; ++q)
> ;
> @@ -437,9 +433,6 @@ static unsigned int __init prom_claim(un
>
> static void __init __attribute__((noreturn)) prom_panic(const char *reason)
> {
> -#ifdef CONFIG_PPC64
> - reason = PTRRELOC(reason);
> -#endif
> prom_print(reason);
> /* Do not call exit because it clears the screen on pmac
> * it also causes some sort of double-fault on early pmacs */
> @@ -929,7 +922,7 @@ static void __init prom_send_capabilitie
> * (we assume this is the same for all cores) and use it to
> * divide NR_CPUS.
> */
> - cores = (u32 *)PTRRELOC(&ibm_architecture_vec[IBM_ARCH_VEC_NRCORES_OFFSET]);
> + cores = (u32 *)&ibm_architecture_vec[IBM_ARCH_VEC_NRCORES_OFFSET];
> if (*cores != NR_CPUS) {
> prom_printf("WARNING ! "
> "ibm_architecture_vec structure inconsistent: %lu!\n",
> @@ -2850,6 +2843,53 @@ static void __init prom_check_initrd(uns
> #endif /* CONFIG_BLK_DEV_INITRD */
> }
>
> +#ifdef CONFIG_PPC64
> +#ifdef CONFIG_RELOCATABLE
> +static void reloc_toc(void)
> +{
> +}
> +
> +static void unreloc_toc(void)
> +{
> +}
> +#else
> +static void __reloc_toc(void *tocstart, unsigned long offset,
> + unsigned long nr_entries)
> +{
> + unsigned long i;
> + unsigned long *toc_entry = (unsigned long *)tocstart;
> +
> + for (i = 0; i < nr_entries; i++) {
> + *toc_entry = *toc_entry + offset;
> + toc_entry++;
> + }
> +}
> +
> +static void reloc_toc(void)
> +{
> + unsigned long offset = reloc_offset();
> + unsigned long nr_entries =
> + (__prom_init_toc_end - __prom_init_toc_start) / sizeof(long);
> +
> + /* Need to add offset to get at __prom_init_toc_start */
> + __reloc_toc(__prom_init_toc_start + offset, offset, nr_entries);
> +
> + mb();
> +}
> +
> +static void unreloc_toc(void)
> +{
> + unsigned long offset = reloc_offset();
> + unsigned long nr_entries =
> + (__prom_init_toc_end - __prom_init_toc_start) / sizeof(long);
> +
> + mb();
> +
> + /* __prom_init_toc_start has been relocated, no need to add offset */
> + __reloc_toc(__prom_init_toc_start, -offset, nr_entries);
> +}
> +#endif
> +#endif
>
> /*
> * We enter here early on, when the Open Firmware prom is still
> @@ -2867,6 +2907,8 @@ unsigned long __init prom_init(unsigned
> #ifdef CONFIG_PPC32
> unsigned long offset = reloc_offset();
> reloc_got2(offset);
> +#else
> + reloc_toc();
> #endif
>
> _prom = &RELOC(prom);
> @@ -3061,6 +3103,8 @@ unsigned long __init prom_init(unsigned
>
> #ifdef CONFIG_PPC32
> reloc_got2(-offset);
> +#else
> + unreloc_toc();
> #endif
>
> #ifdef CONFIG_PPC_EARLY_DEBUG_OPAL
> Index: b/arch/powerpc/kernel/prom_init_check.sh
> ===================================================================
> --- a/arch/powerpc/kernel/prom_init_check.sh
> +++ b/arch/powerpc/kernel/prom_init_check.sh
> @@ -22,7 +22,7 @@ __secondary_hold_acknowledge __secondary
> strcmp strcpy strlcpy strlen strncmp strstr logo_linux_clut224
> reloc_got2 kernstart_addr memstart_addr linux_banner _stext
> opal_query_takeover opal_do_takeover opal_enter_rtas opal_secondary_entry
> -boot_command_line"
> +boot_command_line __prom_init_toc_start __prom_init_toc_end"
>
> NM="$1"
> OBJ="$2"
> Index: b/arch/powerpc/kernel/vmlinux.lds.S
> ===================================================================
> --- a/arch/powerpc/kernel/vmlinux.lds.S
> +++ b/arch/powerpc/kernel/vmlinux.lds.S
> @@ -218,6 +218,11 @@ SECTIONS
>
> .got : AT(ADDR(.got) - LOAD_OFFSET) {
> __toc_start = .;
> +#ifndef CONFIG_RELOCATABLE
> + __prom_init_toc_start = .;
> + arch/powerpc/kernel/prom_init.o*(.toc .got)
> + __prom_init_toc_end = .;
> +#endif
> *(.got)
> *(.toc)
> }
> Index: b/arch/powerpc/Makefile
> ===================================================================
> --- a/arch/powerpc/Makefile
> +++ b/arch/powerpc/Makefile
> @@ -136,6 +136,7 @@ head-$(CONFIG_FSL_BOOKE) := arch/powerpc
> head-$(CONFIG_PPC64) += arch/powerpc/kernel/entry_64.o
> head-$(CONFIG_PPC_FPU) += arch/powerpc/kernel/fpu.o
> head-$(CONFIG_ALTIVEC) += arch/powerpc/kernel/vector.o
> +head-$(CONFIG_PPC_OF_BOOT_TRAMPOLINE) += arch/powerpc/kernel/prom_init.o
>
> core-y += arch/powerpc/kernel/ \
> arch/powerpc/mm/ \
> Index: b/arch/powerpc/kernel/Makefile
> ===================================================================
> --- a/arch/powerpc/kernel/Makefile
> +++ b/arch/powerpc/kernel/Makefile
> @@ -91,7 +91,6 @@ obj-$(CONFIG_RELOCATABLE_PPC32) += reloc
> obj-$(CONFIG_PPC32) += entry_32.o setup_32.o
> obj-$(CONFIG_PPC64) += dma-iommu.o iommu.o
> obj-$(CONFIG_KGDB) += kgdb.o
> -obj-$(CONFIG_PPC_OF_BOOT_TRAMPOLINE) += prom_init.o
> obj-$(CONFIG_MODULES) += ppc_ksyms.o
> obj-$(CONFIG_BOOTX_TEXT) += btext.o
> obj-$(CONFIG_SMP) += smp.o
> @@ -142,6 +141,7 @@ GCOV_PROFILE_kprobes.o := n
> extra-$(CONFIG_PPC_FPU) += fpu.o
> extra-$(CONFIG_ALTIVEC) += vector.o
> extra-$(CONFIG_PPC64) += entry_64.o
> +extra-$(CONFIG_PPC_OF_BOOT_TRAMPOLINE) += prom_init.o
>
> extra-y += systbl_chk.i
> $(obj)/systbl.o: systbl_chk
> Index: b/arch/powerpc/include/asm/sections.h
> ===================================================================
> --- a/arch/powerpc/include/asm/sections.h
> +++ b/arch/powerpc/include/asm/sections.h
> @@ -10,6 +10,9 @@
>
> extern char __end_interrupts[];
>
> +extern char __prom_init_toc_start[];
> +extern char __prom_init_toc_end[];
> +
> static inline int in_kernel_text(unsigned long addr)
> {
> if (addr >= (unsigned long)_stext && addr < (unsigned long)__init_end)
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alex Williamson @ 2012-11-27 5:07 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <50B44866.5000905@ozlabs.ru>
On Tue, 2012-11-27 at 15:58 +1100, Alexey Kardashevskiy wrote:
> On 27/11/12 15:29, Alex Williamson wrote:
> > On Tue, 2012-11-27 at 15:06 +1100, Alexey Kardashevskiy wrote:
> >> On 27/11/12 05:20, Alex Williamson wrote:
> >>> On Fri, 2012-11-23 at 20:03 +1100, Alexey Kardashevskiy wrote:
> >>>> VFIO implements platform independent stuff such as
> >>>> a PCI driver, BAR access (via read/write on a file descriptor
> >>>> or direct mapping when possible) and IRQ signaling.
> >>>>
> >>>> The platform dependent part includes IOMMU initialization
> >>>> and handling. This patch implements an IOMMU driver for VFIO
> >>>> which does mapping/unmapping pages for the guest IO and
> >>>> provides information about DMA window (required by a POWERPC
> >>>> guest).
> >>>>
> >>>> The counterpart in QEMU is required to support this functionality.
> >>>>
> >>>> Cc: David Gibson <david@gibson.dropbear.id.au>
> >>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> ---
> >>>> drivers/vfio/Kconfig | 6 +
> >>>> drivers/vfio/Makefile | 1 +
> >>>> drivers/vfio/vfio_iommu_spapr_tce.c | 247 +++++++++++++++++++++++++++++++++++
> >>>> include/linux/vfio.h | 20 +++
> >>>> 4 files changed, 274 insertions(+)
> >>>> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>
> >>>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> >>>> index 7cd5dec..b464687 100644
> >>>> --- a/drivers/vfio/Kconfig
> >>>> +++ b/drivers/vfio/Kconfig
> >>>> @@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
> >>>> depends on VFIO
> >>>> default n
> >>>>
> >>>> +config VFIO_IOMMU_SPAPR_TCE
> >>>> + tristate
> >>>> + depends on VFIO && SPAPR_TCE_IOMMU
> >>>> + default n
> >>>> +
> >>>> menuconfig VFIO
> >>>> tristate "VFIO Non-Privileged userspace driver framework"
> >>>> depends on IOMMU_API
> >>>> select VFIO_IOMMU_TYPE1 if X86
> >>>> + select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
> >>>> help
> >>>> VFIO provides a framework for secure userspace device drivers.
> >>>> See Documentation/vfio.txt for more details.
> >>>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> >>>> index 2398d4a..72bfabc 100644
> >>>> --- a/drivers/vfio/Makefile
> >>>> +++ b/drivers/vfio/Makefile
> >>>> @@ -1,3 +1,4 @@
> >>>> obj-$(CONFIG_VFIO) += vfio.o
> >>>> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> >>>> +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> >>>> obj-$(CONFIG_VFIO_PCI) += pci/
> >>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> new file mode 100644
> >>>> index 0000000..46a6298
> >>>> --- /dev/null
> >>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>> @@ -0,0 +1,247 @@
> >>>> +/*
> >>>> + * VFIO: IOMMU DMA mapping support for TCE on POWER
> >>>> + *
> >>>> + * Copyright (C) 2012 IBM Corp. All rights reserved.
> >>>> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>> + *
> >>>> + * This program is free software; you can redistribute it and/or modify
> >>>> + * it under the terms of the GNU General Public License version 2 as
> >>>> + * published by the Free Software Foundation.
> >>>> + *
> >>>> + * Derived from original vfio_iommu_type1.c:
> >>>> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> >>>> + * Author: Alex Williamson <alex.williamson@redhat.com>
> >>>> + */
> >>>> +
> >>>> +#include <linux/module.h>
> >>>> +#include <linux/pci.h>
> >>>> +#include <linux/slab.h>
> >>>> +#include <linux/uaccess.h>
> >>>> +#include <linux/err.h>
> >>>> +#include <linux/vfio.h>
> >>>> +#include <asm/iommu.h>
> >>>> +
> >>>> +#define DRIVER_VERSION "0.1"
> >>>> +#define DRIVER_AUTHOR "aik@ozlabs.ru"
> >>>> +#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
> >>>> +
> >>>> +static void tce_iommu_detach_group(void *iommu_data,
> >>>> + struct iommu_group *iommu_group);
> >>>> +
> >>>> +/*
> >>>> + * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
> >>>> + */
> >>>> +
> >>>> +/*
> >>>> + * The container descriptor supports only a single group per container.
> >>>> + * Required by the API as the container is not supplied with the IOMMU group
> >>>> + * at the moment of initialization.
> >>>> + */
> >>>> +struct tce_container {
> >>>> + struct mutex lock;
> >>>> + struct iommu_table *tbl;
> >>>> +};
> >>>> +
> >>>> +static void *tce_iommu_open(unsigned long arg)
> >>>> +{
> >>>> + struct tce_container *container;
> >>>> +
> >>>> + if (arg != VFIO_SPAPR_TCE_IOMMU) {
> >>>> + printk(KERN_ERR "tce_vfio: Wrong IOMMU type\n");
> >>>> + return ERR_PTR(-EINVAL);
> >>>> + }
> >>>> +
> >>>> + container = kzalloc(sizeof(*container), GFP_KERNEL);
> >>>> + if (!container)
> >>>> + return ERR_PTR(-ENOMEM);
> >>>> +
> >>>> + mutex_init(&container->lock);
> >>>> +
> >>>> + return container;
> >>>> +}
> >>>> +
> >>>> +static void tce_iommu_release(void *iommu_data)
> >>>> +{
> >>>> + struct tce_container *container = iommu_data;
> >>>> +
> >>>> + WARN_ON(container->tbl && !container->tbl->it_group);
> >>>
> >>> I think your patch ordering is backwards here. it_group isn't added
> >>> until 2/2. I'd really like to see the arch/powerpc code approved and
> >>> merged by the powerpc maintainer before we add the code that makes use
> >>> of it into vfio. Otherwise we just get lots of churn if interfaces
> >>> change or they disapprove of it altogether.
> >>
> >>
> >> Makes sense, thanks.
> >>
> >>
> >>>> + if (container->tbl && container->tbl->it_group)
> >>>> + tce_iommu_detach_group(iommu_data, container->tbl->it_group);
> >>>> +
> >>>> + mutex_destroy(&container->lock);
> >>>> +
> >>>> + kfree(container);
> >>>> +}
> >>>> +
> >>>> +static long tce_iommu_ioctl(void *iommu_data,
> >>>> + unsigned int cmd, unsigned long arg)
> >>>> +{
> >>>> + struct tce_container *container = iommu_data;
> >>>> + unsigned long minsz;
> >>>> +
> >>>> + switch (cmd) {
> >>>> + case VFIO_CHECK_EXTENSION: {
> >>>> + return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
> >>>> + }
> >>>> + case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
> >>>> + struct vfio_iommu_spapr_tce_info info;
> >>>> + struct iommu_table *tbl = container->tbl;
> >>>> +
> >>>> + if (WARN_ON(!tbl))
> >>>> + return -ENXIO;
> >>>> +
> >>>> + minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
> >>>> + dma64_window_size);
> >>>> +
> >>>> + if (copy_from_user(&info, (void __user *)arg, minsz))
> >>>> + return -EFAULT;
> >>>> +
> >>>> + if (info.argsz < minsz)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
> >>>> + info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
> >>>> + info.dma64_window_start = 0;
> >>>> + info.dma64_window_size = 0;
> >>>> + info.flags = 0;
> >>>> +
> >>>> + if (copy_to_user((void __user *)arg, &info, minsz))
> >>>> + return -EFAULT;
> >>>> +
> >>>> + return 0;
> >>>> + }
> >>>> + case VFIO_IOMMU_MAP_DMA: {
> >>>> + vfio_iommu_spapr_tce_dma_map param;
> >>>> + struct iommu_table *tbl = container->tbl;
> >>>> + enum dma_data_direction direction = DMA_NONE;
> >>>> +
> >>>> + if (WARN_ON(!tbl))
> >>>> + return -ENXIO;
> >>>> +
> >>>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
> >>>> +
> >>>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> >>>> + return -EFAULT;
> >>>> +
> >>>> + if (param.argsz < minsz)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + if ((param.flags & VFIO_DMA_MAP_FLAG_READ) &&
> >>>> + (param.flags & VFIO_DMA_MAP_FLAG_WRITE)) {
> >>>> + direction = DMA_BIDIRECTIONAL;
> >>>> + } else if (param.flags & VFIO_DMA_MAP_FLAG_READ) {
> >>>> + direction = DMA_TO_DEVICE;
> >>>> + } else if (param.flags & VFIO_DMA_MAP_FLAG_WRITE) {
> >>>> + direction = DMA_FROM_DEVICE;
> >>>> + }
> >>>> +
> >>>> + param.size += param.iova & ~IOMMU_PAGE_MASK;
> >>>> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
> >>>
> >>> On x86 we force iova, vaddr, and size to all be aligned to the smallest
> >>> page granularity of the iommu and return -EINVAL if it doesn't fit.
> >>> What does it imply to the user if they're always aligned to work here?
> >>> Won't this interface happily map overlapping entries with no indication
> >>> to the user that the previous mapping is no longer valid?
> >>> Maybe another reason why a combined unmap/map makes me nervous, we have
> >>> to assume the user knows what they're doing.
> >>
> >>
> >> I got used to guests which do know what they are doing so I am pretty calm :)
> >> but ok, I'll move alignment to the QEMU, it makes sense.
> >>
> >>
> >>>> +
> >>>> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
> >>>> + param.vaddr & IOMMU_PAGE_MASK, direction,
> >>>> + param.size >> IOMMU_PAGE_SHIFT);
> >>>> + }
> >>>> + case VFIO_IOMMU_UNMAP_DMA: {
> >>>> + vfio_iommu_spapr_tce_dma_unmap param;
> >>>> + struct iommu_table *tbl = container->tbl;
> >>>> +
> >>>> + if (WARN_ON(!tbl))
> >>>> + return -ENXIO;
> >>>> +
> >>>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
> >>>> +
> >>>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> >>>> + return -EFAULT;
> >>>> +
> >>>> + if (param.argsz < minsz)
> >>>> + return -EINVAL;
> >>>> +
> >>>> + param.size += param.iova & ~IOMMU_PAGE_MASK;
> >>>> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
> >>>> +
> >>>> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
> >>>> + 0, DMA_NONE, param.size >> IOMMU_PAGE_SHIFT);
> >>>> + }
> >>>> + default:
> >>>> + printk(KERN_WARNING "tce_vfio: unexpected cmd %x\n", cmd);
> >>>
> >>> pr_warn
> >>>
> >>>> + }
> >>>> +
> >>>> + return -ENOTTY;
> >>>> +}
> >>>> +
> >>>> +static int tce_iommu_attach_group(void *iommu_data,
> >>>> + struct iommu_group *iommu_group)
> >>>> +{
> >>>> + struct tce_container *container = iommu_data;
> >>>> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
> >>>> +
> >>>> + BUG_ON(!tbl);
> >>>> + mutex_lock(&container->lock);
> >>>> + pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
> >>>> + iommu_group_id(iommu_group), iommu_group);
> >>>> + if (container->tbl) {
> >>>> + printk(KERN_WARNING "tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
> >>>
> >>> pr_warn
> >>>
> >>>> + iommu_group_id(container->tbl->it_group),
> >>>> + iommu_group_id(iommu_group));
> >>>> + mutex_unlock(&container->lock);
> >>>> + return -EBUSY;
> >>>> + }
> >>>> +
> >>>> + container->tbl = tbl;
> >>>
> >>> Would it be too much paranoia to clear all the tce here as you do below
> >>> on detach?
> >>
> >> Guess so. I do unmap on detach() and the guest calls put_tce(0) (i.e.
> >> unmaps) the whole DMA window at the boot time.
> >
> > But that's just one user of this interface, we can't assume they'll all
> > be so agreeable. If any tces were enabled here, a malicious user would
> > have a window to host memory, right? Thanks,
>
>
> But I still release pages on detach(), how can this code be not called on
> the guest exit (normal or crashed)?
What's the initial state? You leave it clean, but who came before you?
Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: implemented IOMMU driver for VFIO
From: David Gibson @ 2012-11-27 5:06 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: linuxppc-dev, Alex Williamson, Paul Mackerras, linux-kernel, kvm
In-Reply-To: <50B44866.5000905@ozlabs.ru>
On Tue, Nov 27, 2012 at 03:58:14PM +1100, Alexey Kardashevskiy wrote:
> On 27/11/12 15:29, Alex Williamson wrote:
> >On Tue, 2012-11-27 at 15:06 +1100, Alexey Kardashevskiy wrote:
> >>On 27/11/12 05:20, Alex Williamson wrote:
> >>>On Fri, 2012-11-23 at 20:03 +1100, Alexey Kardashevskiy wrote:
> >>>>VFIO implements platform independent stuff such as
> >>>>a PCI driver, BAR access (via read/write on a file descriptor
> >>>>or direct mapping when possible) and IRQ signaling.
> >>>>
> >>>>The platform dependent part includes IOMMU initialization
> >>>>and handling. This patch implements an IOMMU driver for VFIO
> >>>>which does mapping/unmapping pages for the guest IO and
> >>>>provides information about DMA window (required by a POWERPC
> >>>>guest).
> >>>>
> >>>>The counterpart in QEMU is required to support this functionality.
> >>>>
> >>>>Cc: David Gibson <david@gibson.dropbear.id.au>
> >>>>Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>---
> >>>> drivers/vfio/Kconfig | 6 +
> >>>> drivers/vfio/Makefile | 1 +
> >>>> drivers/vfio/vfio_iommu_spapr_tce.c | 247 +++++++++++++++++++++++++++++++++++
> >>>> include/linux/vfio.h | 20 +++
> >>>> 4 files changed, 274 insertions(+)
> >>>> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>
> >>>>diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> >>>>index 7cd5dec..b464687 100644
> >>>>--- a/drivers/vfio/Kconfig
> >>>>+++ b/drivers/vfio/Kconfig
> >>>>@@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
> >>>> depends on VFIO
> >>>> default n
> >>>>
> >>>>+config VFIO_IOMMU_SPAPR_TCE
> >>>>+ tristate
> >>>>+ depends on VFIO && SPAPR_TCE_IOMMU
> >>>>+ default n
> >>>>+
> >>>> menuconfig VFIO
> >>>> tristate "VFIO Non-Privileged userspace driver framework"
> >>>> depends on IOMMU_API
> >>>> select VFIO_IOMMU_TYPE1 if X86
> >>>>+ select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
> >>>> help
> >>>> VFIO provides a framework for secure userspace device drivers.
> >>>> See Documentation/vfio.txt for more details.
> >>>>diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> >>>>index 2398d4a..72bfabc 100644
> >>>>--- a/drivers/vfio/Makefile
> >>>>+++ b/drivers/vfio/Makefile
> >>>>@@ -1,3 +1,4 @@
> >>>> obj-$(CONFIG_VFIO) += vfio.o
> >>>> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> >>>>+obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> >>>> obj-$(CONFIG_VFIO_PCI) += pci/
> >>>>diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>new file mode 100644
> >>>>index 0000000..46a6298
> >>>>--- /dev/null
> >>>>+++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>@@ -0,0 +1,247 @@
> >>>>+/*
> >>>>+ * VFIO: IOMMU DMA mapping support for TCE on POWER
> >>>>+ *
> >>>>+ * Copyright (C) 2012 IBM Corp. All rights reserved.
> >>>>+ * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>+ *
> >>>>+ * This program is free software; you can redistribute it and/or modify
> >>>>+ * it under the terms of the GNU General Public License version 2 as
> >>>>+ * published by the Free Software Foundation.
> >>>>+ *
> >>>>+ * Derived from original vfio_iommu_type1.c:
> >>>>+ * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> >>>>+ * Author: Alex Williamson <alex.williamson@redhat.com>
> >>>>+ */
> >>>>+
> >>>>+#include <linux/module.h>
> >>>>+#include <linux/pci.h>
> >>>>+#include <linux/slab.h>
> >>>>+#include <linux/uaccess.h>
> >>>>+#include <linux/err.h>
> >>>>+#include <linux/vfio.h>
> >>>>+#include <asm/iommu.h>
> >>>>+
> >>>>+#define DRIVER_VERSION "0.1"
> >>>>+#define DRIVER_AUTHOR "aik@ozlabs.ru"
> >>>>+#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
> >>>>+
> >>>>+static void tce_iommu_detach_group(void *iommu_data,
> >>>>+ struct iommu_group *iommu_group);
> >>>>+
> >>>>+/*
> >>>>+ * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
> >>>>+ */
> >>>>+
> >>>>+/*
> >>>>+ * The container descriptor supports only a single group per container.
> >>>>+ * Required by the API as the container is not supplied with the IOMMU group
> >>>>+ * at the moment of initialization.
> >>>>+ */
> >>>>+struct tce_container {
> >>>>+ struct mutex lock;
> >>>>+ struct iommu_table *tbl;
> >>>>+};
> >>>>+
> >>>>+static void *tce_iommu_open(unsigned long arg)
> >>>>+{
> >>>>+ struct tce_container *container;
> >>>>+
> >>>>+ if (arg != VFIO_SPAPR_TCE_IOMMU) {
> >>>>+ printk(KERN_ERR "tce_vfio: Wrong IOMMU type\n");
> >>>>+ return ERR_PTR(-EINVAL);
> >>>>+ }
> >>>>+
> >>>>+ container = kzalloc(sizeof(*container), GFP_KERNEL);
> >>>>+ if (!container)
> >>>>+ return ERR_PTR(-ENOMEM);
> >>>>+
> >>>>+ mutex_init(&container->lock);
> >>>>+
> >>>>+ return container;
> >>>>+}
> >>>>+
> >>>>+static void tce_iommu_release(void *iommu_data)
> >>>>+{
> >>>>+ struct tce_container *container = iommu_data;
> >>>>+
> >>>>+ WARN_ON(container->tbl && !container->tbl->it_group);
> >>>
> >>>I think your patch ordering is backwards here. it_group isn't added
> >>>until 2/2. I'd really like to see the arch/powerpc code approved and
> >>>merged by the powerpc maintainer before we add the code that makes use
> >>>of it into vfio. Otherwise we just get lots of churn if interfaces
> >>>change or they disapprove of it altogether.
> >>
> >>
> >>Makes sense, thanks.
> >>
> >>
> >>>>+ if (container->tbl && container->tbl->it_group)
> >>>>+ tce_iommu_detach_group(iommu_data, container->tbl->it_group);
> >>>>+
> >>>>+ mutex_destroy(&container->lock);
> >>>>+
> >>>>+ kfree(container);
> >>>>+}
> >>>>+
> >>>>+static long tce_iommu_ioctl(void *iommu_data,
> >>>>+ unsigned int cmd, unsigned long arg)
> >>>>+{
> >>>>+ struct tce_container *container = iommu_data;
> >>>>+ unsigned long minsz;
> >>>>+
> >>>>+ switch (cmd) {
> >>>>+ case VFIO_CHECK_EXTENSION: {
> >>>>+ return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
> >>>>+ }
> >>>>+ case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
> >>>>+ struct vfio_iommu_spapr_tce_info info;
> >>>>+ struct iommu_table *tbl = container->tbl;
> >>>>+
> >>>>+ if (WARN_ON(!tbl))
> >>>>+ return -ENXIO;
> >>>>+
> >>>>+ minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
> >>>>+ dma64_window_size);
> >>>>+
> >>>>+ if (copy_from_user(&info, (void __user *)arg, minsz))
> >>>>+ return -EFAULT;
> >>>>+
> >>>>+ if (info.argsz < minsz)
> >>>>+ return -EINVAL;
> >>>>+
> >>>>+ info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
> >>>>+ info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
> >>>>+ info.dma64_window_start = 0;
> >>>>+ info.dma64_window_size = 0;
> >>>>+ info.flags = 0;
> >>>>+
> >>>>+ if (copy_to_user((void __user *)arg, &info, minsz))
> >>>>+ return -EFAULT;
> >>>>+
> >>>>+ return 0;
> >>>>+ }
> >>>>+ case VFIO_IOMMU_MAP_DMA: {
> >>>>+ vfio_iommu_spapr_tce_dma_map param;
> >>>>+ struct iommu_table *tbl = container->tbl;
> >>>>+ enum dma_data_direction direction = DMA_NONE;
> >>>>+
> >>>>+ if (WARN_ON(!tbl))
> >>>>+ return -ENXIO;
> >>>>+
> >>>>+ minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
> >>>>+
> >>>>+ if (copy_from_user(¶m, (void __user *)arg, minsz))
> >>>>+ return -EFAULT;
> >>>>+
> >>>>+ if (param.argsz < minsz)
> >>>>+ return -EINVAL;
> >>>>+
> >>>>+ if ((param.flags & VFIO_DMA_MAP_FLAG_READ) &&
> >>>>+ (param.flags & VFIO_DMA_MAP_FLAG_WRITE)) {
> >>>>+ direction = DMA_BIDIRECTIONAL;
> >>>>+ } else if (param.flags & VFIO_DMA_MAP_FLAG_READ) {
> >>>>+ direction = DMA_TO_DEVICE;
> >>>>+ } else if (param.flags & VFIO_DMA_MAP_FLAG_WRITE) {
> >>>>+ direction = DMA_FROM_DEVICE;
> >>>>+ }
> >>>>+
> >>>>+ param.size += param.iova & ~IOMMU_PAGE_MASK;
> >>>>+ param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
> >>>
> >>>On x86 we force iova, vaddr, and size to all be aligned to the smallest
> >>>page granularity of the iommu and return -EINVAL if it doesn't fit.
> >>>What does it imply to the user if they're always aligned to work here?
> >>>Won't this interface happily map overlapping entries with no indication
> >>>to the user that the previous mapping is no longer valid?
> >>>Maybe another reason why a combined unmap/map makes me nervous, we have
> >>>to assume the user knows what they're doing.
> >>
> >>
> >>I got used to guests which do know what they are doing so I am pretty calm :)
> >>but ok, I'll move alignment to the QEMU, it makes sense.
> >>
> >>
> >>>>+
> >>>>+ return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
> >>>>+ param.vaddr & IOMMU_PAGE_MASK, direction,
> >>>>+ param.size >> IOMMU_PAGE_SHIFT);
> >>>>+ }
> >>>>+ case VFIO_IOMMU_UNMAP_DMA: {
> >>>>+ vfio_iommu_spapr_tce_dma_unmap param;
> >>>>+ struct iommu_table *tbl = container->tbl;
> >>>>+
> >>>>+ if (WARN_ON(!tbl))
> >>>>+ return -ENXIO;
> >>>>+
> >>>>+ minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
> >>>>+
> >>>>+ if (copy_from_user(¶m, (void __user *)arg, minsz))
> >>>>+ return -EFAULT;
> >>>>+
> >>>>+ if (param.argsz < minsz)
> >>>>+ return -EINVAL;
> >>>>+
> >>>>+ param.size += param.iova & ~IOMMU_PAGE_MASK;
> >>>>+ param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
> >>>>+
> >>>>+ return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
> >>>>+ 0, DMA_NONE, param.size >> IOMMU_PAGE_SHIFT);
> >>>>+ }
> >>>>+ default:
> >>>>+ printk(KERN_WARNING "tce_vfio: unexpected cmd %x\n", cmd);
> >>>
> >>>pr_warn
> >>>
> >>>>+ }
> >>>>+
> >>>>+ return -ENOTTY;
> >>>>+}
> >>>>+
> >>>>+static int tce_iommu_attach_group(void *iommu_data,
> >>>>+ struct iommu_group *iommu_group)
> >>>>+{
> >>>>+ struct tce_container *container = iommu_data;
> >>>>+ struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
> >>>>+
> >>>>+ BUG_ON(!tbl);
> >>>>+ mutex_lock(&container->lock);
> >>>>+ pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
> >>>>+ iommu_group_id(iommu_group), iommu_group);
> >>>>+ if (container->tbl) {
> >>>>+ printk(KERN_WARNING "tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
> >>>
> >>>pr_warn
> >>>
> >>>>+ iommu_group_id(container->tbl->it_group),
> >>>>+ iommu_group_id(iommu_group));
> >>>>+ mutex_unlock(&container->lock);
> >>>>+ return -EBUSY;
> >>>>+ }
> >>>>+
> >>>>+ container->tbl = tbl;
> >>>
> >>>Would it be too much paranoia to clear all the tce here as you do below
> >>>on detach?
> >>
> >>Guess so. I do unmap on detach() and the guest calls put_tce(0) (i.e.
> >>unmaps) the whole DMA window at the boot time.
> >
> >But that's just one user of this interface, we can't assume they'll all
> >be so agreeable. If any tces were enabled here, a malicious user would
> >have a window to host memory, right? Thanks,
>
>
> But I still release pages on detach(), how can this code be not
> called on the guest exit (normal or crashed)?
I think the concern is about robustness if some bug elsewhere in the
kernel left some TCE entries in place before the table was handed over
to VFIO.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alexey Kardashevskiy @ 2012-11-27 4:58 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1353990588.1809.148.camel@bling.home>
On 27/11/12 15:29, Alex Williamson wrote:
> On Tue, 2012-11-27 at 15:06 +1100, Alexey Kardashevskiy wrote:
>> On 27/11/12 05:20, Alex Williamson wrote:
>>> On Fri, 2012-11-23 at 20:03 +1100, Alexey Kardashevskiy wrote:
>>>> VFIO implements platform independent stuff such as
>>>> a PCI driver, BAR access (via read/write on a file descriptor
>>>> or direct mapping when possible) and IRQ signaling.
>>>>
>>>> The platform dependent part includes IOMMU initialization
>>>> and handling. This patch implements an IOMMU driver for VFIO
>>>> which does mapping/unmapping pages for the guest IO and
>>>> provides information about DMA window (required by a POWERPC
>>>> guest).
>>>>
>>>> The counterpart in QEMU is required to support this functionality.
>>>>
>>>> Cc: David Gibson <david@gibson.dropbear.id.au>
>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> ---
>>>> drivers/vfio/Kconfig | 6 +
>>>> drivers/vfio/Makefile | 1 +
>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 247 +++++++++++++++++++++++++++++++++++
>>>> include/linux/vfio.h | 20 +++
>>>> 4 files changed, 274 insertions(+)
>>>> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
>>>>
>>>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>>>> index 7cd5dec..b464687 100644
>>>> --- a/drivers/vfio/Kconfig
>>>> +++ b/drivers/vfio/Kconfig
>>>> @@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
>>>> depends on VFIO
>>>> default n
>>>>
>>>> +config VFIO_IOMMU_SPAPR_TCE
>>>> + tristate
>>>> + depends on VFIO && SPAPR_TCE_IOMMU
>>>> + default n
>>>> +
>>>> menuconfig VFIO
>>>> tristate "VFIO Non-Privileged userspace driver framework"
>>>> depends on IOMMU_API
>>>> select VFIO_IOMMU_TYPE1 if X86
>>>> + select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
>>>> help
>>>> VFIO provides a framework for secure userspace device drivers.
>>>> See Documentation/vfio.txt for more details.
>>>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
>>>> index 2398d4a..72bfabc 100644
>>>> --- a/drivers/vfio/Makefile
>>>> +++ b/drivers/vfio/Makefile
>>>> @@ -1,3 +1,4 @@
>>>> obj-$(CONFIG_VFIO) += vfio.o
>>>> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>>>> +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
>>>> obj-$(CONFIG_VFIO_PCI) += pci/
>>>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> new file mode 100644
>>>> index 0000000..46a6298
>>>> --- /dev/null
>>>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>>>> @@ -0,0 +1,247 @@
>>>> +/*
>>>> + * VFIO: IOMMU DMA mapping support for TCE on POWER
>>>> + *
>>>> + * Copyright (C) 2012 IBM Corp. All rights reserved.
>>>> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
>>>> + *
>>>> + * This program is free software; you can redistribute it and/or modify
>>>> + * it under the terms of the GNU General Public License version 2 as
>>>> + * published by the Free Software Foundation.
>>>> + *
>>>> + * Derived from original vfio_iommu_type1.c:
>>>> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
>>>> + * Author: Alex Williamson <alex.williamson@redhat.com>
>>>> + */
>>>> +
>>>> +#include <linux/module.h>
>>>> +#include <linux/pci.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/uaccess.h>
>>>> +#include <linux/err.h>
>>>> +#include <linux/vfio.h>
>>>> +#include <asm/iommu.h>
>>>> +
>>>> +#define DRIVER_VERSION "0.1"
>>>> +#define DRIVER_AUTHOR "aik@ozlabs.ru"
>>>> +#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
>>>> +
>>>> +static void tce_iommu_detach_group(void *iommu_data,
>>>> + struct iommu_group *iommu_group);
>>>> +
>>>> +/*
>>>> + * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
>>>> + */
>>>> +
>>>> +/*
>>>> + * The container descriptor supports only a single group per container.
>>>> + * Required by the API as the container is not supplied with the IOMMU group
>>>> + * at the moment of initialization.
>>>> + */
>>>> +struct tce_container {
>>>> + struct mutex lock;
>>>> + struct iommu_table *tbl;
>>>> +};
>>>> +
>>>> +static void *tce_iommu_open(unsigned long arg)
>>>> +{
>>>> + struct tce_container *container;
>>>> +
>>>> + if (arg != VFIO_SPAPR_TCE_IOMMU) {
>>>> + printk(KERN_ERR "tce_vfio: Wrong IOMMU type\n");
>>>> + return ERR_PTR(-EINVAL);
>>>> + }
>>>> +
>>>> + container = kzalloc(sizeof(*container), GFP_KERNEL);
>>>> + if (!container)
>>>> + return ERR_PTR(-ENOMEM);
>>>> +
>>>> + mutex_init(&container->lock);
>>>> +
>>>> + return container;
>>>> +}
>>>> +
>>>> +static void tce_iommu_release(void *iommu_data)
>>>> +{
>>>> + struct tce_container *container = iommu_data;
>>>> +
>>>> + WARN_ON(container->tbl && !container->tbl->it_group);
>>>
>>> I think your patch ordering is backwards here. it_group isn't added
>>> until 2/2. I'd really like to see the arch/powerpc code approved and
>>> merged by the powerpc maintainer before we add the code that makes use
>>> of it into vfio. Otherwise we just get lots of churn if interfaces
>>> change or they disapprove of it altogether.
>>
>>
>> Makes sense, thanks.
>>
>>
>>>> + if (container->tbl && container->tbl->it_group)
>>>> + tce_iommu_detach_group(iommu_data, container->tbl->it_group);
>>>> +
>>>> + mutex_destroy(&container->lock);
>>>> +
>>>> + kfree(container);
>>>> +}
>>>> +
>>>> +static long tce_iommu_ioctl(void *iommu_data,
>>>> + unsigned int cmd, unsigned long arg)
>>>> +{
>>>> + struct tce_container *container = iommu_data;
>>>> + unsigned long minsz;
>>>> +
>>>> + switch (cmd) {
>>>> + case VFIO_CHECK_EXTENSION: {
>>>> + return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
>>>> + }
>>>> + case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
>>>> + struct vfio_iommu_spapr_tce_info info;
>>>> + struct iommu_table *tbl = container->tbl;
>>>> +
>>>> + if (WARN_ON(!tbl))
>>>> + return -ENXIO;
>>>> +
>>>> + minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
>>>> + dma64_window_size);
>>>> +
>>>> + if (copy_from_user(&info, (void __user *)arg, minsz))
>>>> + return -EFAULT;
>>>> +
>>>> + if (info.argsz < minsz)
>>>> + return -EINVAL;
>>>> +
>>>> + info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
>>>> + info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
>>>> + info.dma64_window_start = 0;
>>>> + info.dma64_window_size = 0;
>>>> + info.flags = 0;
>>>> +
>>>> + if (copy_to_user((void __user *)arg, &info, minsz))
>>>> + return -EFAULT;
>>>> +
>>>> + return 0;
>>>> + }
>>>> + case VFIO_IOMMU_MAP_DMA: {
>>>> + vfio_iommu_spapr_tce_dma_map param;
>>>> + struct iommu_table *tbl = container->tbl;
>>>> + enum dma_data_direction direction = DMA_NONE;
>>>> +
>>>> + if (WARN_ON(!tbl))
>>>> + return -ENXIO;
>>>> +
>>>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
>>>> +
>>>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
>>>> + return -EFAULT;
>>>> +
>>>> + if (param.argsz < minsz)
>>>> + return -EINVAL;
>>>> +
>>>> + if ((param.flags & VFIO_DMA_MAP_FLAG_READ) &&
>>>> + (param.flags & VFIO_DMA_MAP_FLAG_WRITE)) {
>>>> + direction = DMA_BIDIRECTIONAL;
>>>> + } else if (param.flags & VFIO_DMA_MAP_FLAG_READ) {
>>>> + direction = DMA_TO_DEVICE;
>>>> + } else if (param.flags & VFIO_DMA_MAP_FLAG_WRITE) {
>>>> + direction = DMA_FROM_DEVICE;
>>>> + }
>>>> +
>>>> + param.size += param.iova & ~IOMMU_PAGE_MASK;
>>>> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
>>>
>>> On x86 we force iova, vaddr, and size to all be aligned to the smallest
>>> page granularity of the iommu and return -EINVAL if it doesn't fit.
>>> What does it imply to the user if they're always aligned to work here?
>>> Won't this interface happily map overlapping entries with no indication
>>> to the user that the previous mapping is no longer valid?
>>> Maybe another reason why a combined unmap/map makes me nervous, we have
>>> to assume the user knows what they're doing.
>>
>>
>> I got used to guests which do know what they are doing so I am pretty calm :)
>> but ok, I'll move alignment to the QEMU, it makes sense.
>>
>>
>>>> +
>>>> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
>>>> + param.vaddr & IOMMU_PAGE_MASK, direction,
>>>> + param.size >> IOMMU_PAGE_SHIFT);
>>>> + }
>>>> + case VFIO_IOMMU_UNMAP_DMA: {
>>>> + vfio_iommu_spapr_tce_dma_unmap param;
>>>> + struct iommu_table *tbl = container->tbl;
>>>> +
>>>> + if (WARN_ON(!tbl))
>>>> + return -ENXIO;
>>>> +
>>>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
>>>> +
>>>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
>>>> + return -EFAULT;
>>>> +
>>>> + if (param.argsz < minsz)
>>>> + return -EINVAL;
>>>> +
>>>> + param.size += param.iova & ~IOMMU_PAGE_MASK;
>>>> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
>>>> +
>>>> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
>>>> + 0, DMA_NONE, param.size >> IOMMU_PAGE_SHIFT);
>>>> + }
>>>> + default:
>>>> + printk(KERN_WARNING "tce_vfio: unexpected cmd %x\n", cmd);
>>>
>>> pr_warn
>>>
>>>> + }
>>>> +
>>>> + return -ENOTTY;
>>>> +}
>>>> +
>>>> +static int tce_iommu_attach_group(void *iommu_data,
>>>> + struct iommu_group *iommu_group)
>>>> +{
>>>> + struct tce_container *container = iommu_data;
>>>> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
>>>> +
>>>> + BUG_ON(!tbl);
>>>> + mutex_lock(&container->lock);
>>>> + pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
>>>> + iommu_group_id(iommu_group), iommu_group);
>>>> + if (container->tbl) {
>>>> + printk(KERN_WARNING "tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
>>>
>>> pr_warn
>>>
>>>> + iommu_group_id(container->tbl->it_group),
>>>> + iommu_group_id(iommu_group));
>>>> + mutex_unlock(&container->lock);
>>>> + return -EBUSY;
>>>> + }
>>>> +
>>>> + container->tbl = tbl;
>>>
>>> Would it be too much paranoia to clear all the tce here as you do below
>>> on detach?
>>
>> Guess so. I do unmap on detach() and the guest calls put_tce(0) (i.e.
>> unmaps) the whole DMA window at the boot time.
>
> But that's just one user of this interface, we can't assume they'll all
> be so agreeable. If any tces were enabled here, a malicious user would
> have a window to host memory, right? Thanks,
But I still release pages on detach(), how can this code be not called on
the guest exit (normal or crashed)?
>
> Alex
>
--
Alexey
^ permalink raw reply
* Re: [PATCH 2/2] vfio powerpc: enabled on powernv platform
From: Alex Williamson @ 2012-11-27 4:41 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1353661396-14374-3-git-send-email-aik@ozlabs.ru>
On Fri, 2012-11-23 at 20:03 +1100, Alexey Kardashevskiy wrote:
> This patch initializes IOMMU groups based on the IOMMU
> configuration discovered during the PCI scan on POWERNV
> (POWER non virtualized) platform. The IOMMU groups are
> to be used later by VFIO driver (PCI pass through).
>
> It also implements an API for mapping/unmapping pages for
> guest PCI drivers and providing DMA window properties.
> This API is going to be used later by QEMU-VFIO to handle
> h_put_tce hypercalls from the KVM guest.
>
> Although this driver has been tested only on the POWERNV
> platform, it should work on any platform which supports
> TCE tables.
>
> To enable VFIO on POWER, enable SPAPR_TCE_IOMMU config
> option and configure VFIO as required.
>
> Cc: David Gibson <david@gibson.dropbear.id.au>
> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> ---
> arch/powerpc/include/asm/iommu.h | 6 ++
> arch/powerpc/kernel/iommu.c | 141 ++++++++++++++++++++++++++++++++++
> arch/powerpc/platforms/powernv/pci.c | 135 ++++++++++++++++++++++++++++++++
> drivers/iommu/Kconfig | 8 ++
> 4 files changed, 290 insertions(+)
>
> diff --git a/arch/powerpc/include/asm/iommu.h b/arch/powerpc/include/asm/iommu.h
> index cbfe678..5ba66cb 100644
> --- a/arch/powerpc/include/asm/iommu.h
> +++ b/arch/powerpc/include/asm/iommu.h
> @@ -76,6 +76,9 @@ struct iommu_table {
> struct iommu_pool large_pool;
> struct iommu_pool pools[IOMMU_NR_POOLS];
> unsigned long *it_map; /* A simple allocation bitmap for now */
> +#ifdef CONFIG_IOMMU_API
> + struct iommu_group *it_group;
> +#endif
> };
>
> struct scatterlist;
> @@ -147,5 +150,8 @@ static inline void iommu_restore(void)
> }
> #endif
>
> +extern long iommu_put_tces(struct iommu_table *tbl, unsigned long entry, uint64_t tce,
> + enum dma_data_direction direction, unsigned long pages);
> +
> #endif /* __KERNEL__ */
> #endif /* _ASM_IOMMU_H */
> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> index ff5a6ce..c8dad1f 100644
> --- a/arch/powerpc/kernel/iommu.c
> +++ b/arch/powerpc/kernel/iommu.c
> @@ -44,6 +44,7 @@
> #include <asm/kdump.h>
> #include <asm/fadump.h>
> #include <asm/vio.h>
> +#include <asm/tce.h>
>
> #define DBG(...)
>
> @@ -856,3 +857,143 @@ void iommu_free_coherent(struct iommu_table *tbl, size_t size,
> free_pages((unsigned long)vaddr, get_order(size));
> }
> }
> +
> +#ifdef CONFIG_IOMMU_API
> +/*
> + * SPAPR TCE API
> + */
> +static struct page *free_tce(struct iommu_table *tbl, unsigned long entry)
> +{
> + struct page *page;
> + unsigned long oldtce;
> +
> + oldtce = ppc_md.tce_get(tbl, entry);
> +
> + if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> + return NULL;
> +
> + page = pfn_to_page(oldtce >> PAGE_SHIFT);
> +
> + WARN_ON(!page);
> + if (page && (oldtce & TCE_PCI_WRITE))
> + SetPageDirty(page);
> + ppc_md.tce_free(tbl, entry, 1);
> +
> + return page;
> +}
> +
> +static int put_tce(struct iommu_table *tbl, unsigned long entry,
> + uint64_t tce, enum dma_data_direction direction)
> +{
> + int ret;
> + struct page *page = NULL;
> + unsigned long kva, offset;
> +
> + /* Map new TCE */
> + offset = (tce & IOMMU_PAGE_MASK) - (tce & PAGE_MASK);
> + ret = get_user_pages_fast(tce & PAGE_MASK, 1,
> + direction != DMA_TO_DEVICE, &page);
We're locking memory here on behalf of the user, but I don't see where
rlimit gets checked to verify the user has privileges to lock the pages.
I know you're locking a much smaller set of memory than x86 does, but
are we just foregoing that added security?
> + if (ret < 1) {
> + printk(KERN_ERR "tce_vfio: get_user_pages_fast failed tce=%llx ioba=%lx ret=%d\n",
> + tce, entry << IOMMU_PAGE_SHIFT, ret);
> + if (!ret)
> + ret = -EFAULT;
> + return ret;
> + }
> +
> + kva = (unsigned long) page_address(page);
> + kva += offset;
> +
> + /* tce_build receives a virtual address */
> + entry += tbl->it_offset; /* Offset into real TCE table */
> + ret = ppc_md.tce_build(tbl, entry, 1, kva, direction, NULL);
> +
> + /* tce_build() only returns non-zero for transient errors */
> + if (unlikely(ret)) {
> + printk(KERN_ERR "tce_vfio: tce_put failed on tce=%llx ioba=%lx kva=%lx ret=%d\n",
> + tce, entry << IOMMU_PAGE_SHIFT, kva, ret);
> + put_page(page);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static void tce_flush(struct iommu_table *tbl)
> +{
> + /* Flush/invalidate TLB caches if necessary */
> + if (ppc_md.tce_flush)
> + ppc_md.tce_flush(tbl);
> +
> + /* Make sure updates are seen by hardware */
> + mb();
> +}
> +
> +long iommu_put_tces(struct iommu_table *tbl, unsigned long entry, uint64_t tce,
> + enum dma_data_direction direction, unsigned long pages)
> +{
> + int i, ret = 0, pages_to_put = 0;
> + struct page *page;
> + struct iommu_pool *pool = get_pool(tbl, entry);
> + struct page **oldpages;
> + const int oldpagesnum = PAGE_SIZE/sizeof(*oldpages);
> +
> + BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> +
> + /* Handle a single page request without allocation
> + of pages-to-release array */
nit, this comment style doesn't seem to match anything existing in this
file. I'd also be tempted to use pr_err/warn in this file, but I'll
leave that for the maintainers. Thanks,
Alex
> + if (pages == 1) {
> + spin_lock(&(pool->lock));
> + page = free_tce(tbl, entry);
> +
> + if (direction != DMA_NONE)
> + ret = put_tce(tbl, entry, tce, direction);
> +
> + tce_flush(tbl);
> +
> + if (page)
> + put_page(page);
> +
> + spin_unlock(&(pool->lock));
> + return ret;
> + }
> +
> + /* Releasing multiple pages */
> + /* Allocate an array for pages to be released after TCE table
> + is updated */
> + oldpages = kmalloc(PAGE_SIZE, GFP_KERNEL);
> + if (!oldpages)
> + return -ENOMEM;
> +
> + spin_lock(&(pool->lock));
> +
> + for (i = 0; (i < pages) && !ret; ++i, ++entry, tce += IOMMU_PAGE_SIZE) {
> + page = free_tce(tbl, entry);
> + if (page) {
> + oldpages[pages_to_put] = page;
> + ++pages_to_put;
> + }
> +
> + if (direction != DMA_NONE)
> + ret = put_tce(tbl, entry, tce, direction);
> +
> + /* Release old pages if we reached the end of oldpages[] or
> + it is the last page or we are about to exit the loop */
> + if ((pages_to_put == oldpagesnum) || (i == pages - 1) || ret) {
> + tce_flush(tbl);
> +
> + /* Release pages after removing them from TCE table */
> + while (pages_to_put) {
> + --pages_to_put;
> + put_page(oldpages[pages_to_put]);
> + }
> + }
> + }
> +
> + spin_unlock(&(pool->lock));
> + kfree(oldpages);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(iommu_put_tces);
> +#endif /* CONFIG_IOMMU_API */
> diff --git a/arch/powerpc/platforms/powernv/pci.c b/arch/powerpc/platforms/powernv/pci.c
> index 05205cf..660dcc6 100644
> --- a/arch/powerpc/platforms/powernv/pci.c
> +++ b/arch/powerpc/platforms/powernv/pci.c
> @@ -20,6 +20,7 @@
> #include <linux/irq.h>
> #include <linux/io.h>
> #include <linux/msi.h>
> +#include <linux/iommu.h>
>
> #include <asm/sections.h>
> #include <asm/io.h>
> @@ -613,3 +614,137 @@ void __init pnv_pci_init(void)
> ppc_md.teardown_msi_irqs = pnv_teardown_msi_irqs;
> #endif
> }
> +
> +#ifdef CONFIG_IOMMU_API
> +/*
> + * IOMMU groups support required by VFIO
> + */
> +static int add_device(struct device *dev)
> +{
> + struct iommu_table *tbl;
> + int ret = 0;
> +
> + if (WARN_ON(dev->iommu_group)) {
> + printk(KERN_WARNING "tce_vfio: device %s is already in iommu group %d, skipping\n",
> + dev_name(dev),
> + iommu_group_id(dev->iommu_group));
> + return -EBUSY;
> + }
> +
> + tbl = get_iommu_table_base(dev);
> + if (!tbl) {
> + pr_debug("tce_vfio: skipping device %s with no tbl\n",
> + dev_name(dev));
> + return 0;
> + }
> +
> + pr_debug("tce_vfio: adding %s to iommu group %d\n",
> + dev_name(dev), iommu_group_id(tbl->it_group));
> +
> + ret = iommu_group_add_device(tbl->it_group, dev);
> + if (ret < 0)
> + printk(KERN_ERR "tce_vfio: %s has not been added, ret=%d\n",
> + dev_name(dev), ret);
> +
> + return ret;
> +}
> +
> +static void del_device(struct device *dev)
> +{
> + iommu_group_remove_device(dev);
> +}
> +
> +static int iommu_bus_notifier(struct notifier_block *nb,
> + unsigned long action, void *data)
> +{
> + struct device *dev = data;
> +
> + switch (action) {
> + case BUS_NOTIFY_ADD_DEVICE:
> + return add_device(dev);
> + case BUS_NOTIFY_DEL_DEVICE:
> + del_device(dev);
> + return 0;
> + default:
> + return 0;
> + }
> +}
> +
> +static struct notifier_block tce_iommu_bus_nb = {
> + .notifier_call = iommu_bus_notifier,
> +};
> +
> +static void group_release(void *iommu_data)
> +{
> + struct iommu_table *tbl = iommu_data;
> + tbl->it_group = NULL;
> +}
> +
> +static int __init tce_iommu_init(void)
> +{
> + struct pci_dev *pdev = NULL;
> + struct iommu_table *tbl;
> + struct iommu_group *grp;
> +
> + bus_register_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> +
> + /* Allocate and initialize IOMMU groups */
> + for_each_pci_dev(pdev) {
> + tbl = get_iommu_table_base(&pdev->dev);
> + if (!tbl)
> + continue;
> +
> + /* Skip already initialized */
> + if (tbl->it_group)
> + continue;
> +
> + grp = iommu_group_alloc();
> + if (IS_ERR(grp)) {
> + printk(KERN_INFO "tce_vfio: cannot create "
> + "new IOMMU group, ret=%ld\n",
> + PTR_ERR(grp));
> + return PTR_ERR(grp);
> + }
> + tbl->it_group = grp;
> + iommu_group_set_iommudata(grp, tbl, group_release);
> + }
> +
> + /* Add PCI devices to VFIO groups */
> + for_each_pci_dev(pdev)
> + add_device(&pdev->dev);
> +
> + return 0;
> +}
> +
> +static void __exit tce_iommu_cleanup(void)
> +{
> + struct pci_dev *pdev = NULL;
> + struct iommu_table *tbl;
> + struct iommu_group *grp = NULL;
> +
> + bus_unregister_notifier(&pci_bus_type, &tce_iommu_bus_nb);
> +
> + /* Delete PCI devices from VFIO groups */
> + for_each_pci_dev(pdev)
> + del_device(&pdev->dev);
> +
> + /* Release VFIO groups */
> + for_each_pci_dev(pdev) {
> + tbl = get_iommu_table_base(&pdev->dev);
> + if (!tbl)
> + continue;
> + grp = tbl->it_group;
> +
> + /* Skip (already) uninitialized */
> + if (!grp)
> + continue;
> +
> + /* Do actual release, group_release() is expected to work */
> + iommu_group_put(grp);
> + BUG_ON(tbl->it_group);
> + }
> +}
> +
> +module_init(tce_iommu_init);
> +module_exit(tce_iommu_cleanup);
> +#endif /* CONFIG_IOMMU_API */
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 9f69b56..29d11dc 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -187,4 +187,12 @@ config EXYNOS_IOMMU_DEBUG
>
> Say N unless you need kernel log message for IOMMU debugging
>
> +config SPAPR_TCE_IOMMU
> + bool "sPAPR TCE IOMMU Support"
> + depends on PPC_POWERNV
> + select IOMMU_API
> + help
> + Enables bits of IOMMU API required by VFIO. The iommu_ops is
> + still not implemented.
> +
> endif # IOMMU_SUPPORT
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alex Williamson @ 2012-11-27 4:29 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <50B43C36.60707@ozlabs.ru>
On Tue, 2012-11-27 at 15:06 +1100, Alexey Kardashevskiy wrote:
> On 27/11/12 05:20, Alex Williamson wrote:
> > On Fri, 2012-11-23 at 20:03 +1100, Alexey Kardashevskiy wrote:
> >> VFIO implements platform independent stuff such as
> >> a PCI driver, BAR access (via read/write on a file descriptor
> >> or direct mapping when possible) and IRQ signaling.
> >>
> >> The platform dependent part includes IOMMU initialization
> >> and handling. This patch implements an IOMMU driver for VFIO
> >> which does mapping/unmapping pages for the guest IO and
> >> provides information about DMA window (required by a POWERPC
> >> guest).
> >>
> >> The counterpart in QEMU is required to support this functionality.
> >>
> >> Cc: David Gibson <david@gibson.dropbear.id.au>
> >> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> ---
> >> drivers/vfio/Kconfig | 6 +
> >> drivers/vfio/Makefile | 1 +
> >> drivers/vfio/vfio_iommu_spapr_tce.c | 247 +++++++++++++++++++++++++++++++++++
> >> include/linux/vfio.h | 20 +++
> >> 4 files changed, 274 insertions(+)
> >> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
> >>
> >> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
> >> index 7cd5dec..b464687 100644
> >> --- a/drivers/vfio/Kconfig
> >> +++ b/drivers/vfio/Kconfig
> >> @@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
> >> depends on VFIO
> >> default n
> >>
> >> +config VFIO_IOMMU_SPAPR_TCE
> >> + tristate
> >> + depends on VFIO && SPAPR_TCE_IOMMU
> >> + default n
> >> +
> >> menuconfig VFIO
> >> tristate "VFIO Non-Privileged userspace driver framework"
> >> depends on IOMMU_API
> >> select VFIO_IOMMU_TYPE1 if X86
> >> + select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
> >> help
> >> VFIO provides a framework for secure userspace device drivers.
> >> See Documentation/vfio.txt for more details.
> >> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
> >> index 2398d4a..72bfabc 100644
> >> --- a/drivers/vfio/Makefile
> >> +++ b/drivers/vfio/Makefile
> >> @@ -1,3 +1,4 @@
> >> obj-$(CONFIG_VFIO) += vfio.o
> >> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
> >> +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
> >> obj-$(CONFIG_VFIO_PCI) += pci/
> >> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> new file mode 100644
> >> index 0000000..46a6298
> >> --- /dev/null
> >> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
> >> @@ -0,0 +1,247 @@
> >> +/*
> >> + * VFIO: IOMMU DMA mapping support for TCE on POWER
> >> + *
> >> + * Copyright (C) 2012 IBM Corp. All rights reserved.
> >> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
> >> + *
> >> + * This program is free software; you can redistribute it and/or modify
> >> + * it under the terms of the GNU General Public License version 2 as
> >> + * published by the Free Software Foundation.
> >> + *
> >> + * Derived from original vfio_iommu_type1.c:
> >> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
> >> + * Author: Alex Williamson <alex.williamson@redhat.com>
> >> + */
> >> +
> >> +#include <linux/module.h>
> >> +#include <linux/pci.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/uaccess.h>
> >> +#include <linux/err.h>
> >> +#include <linux/vfio.h>
> >> +#include <asm/iommu.h>
> >> +
> >> +#define DRIVER_VERSION "0.1"
> >> +#define DRIVER_AUTHOR "aik@ozlabs.ru"
> >> +#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
> >> +
> >> +static void tce_iommu_detach_group(void *iommu_data,
> >> + struct iommu_group *iommu_group);
> >> +
> >> +/*
> >> + * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
> >> + */
> >> +
> >> +/*
> >> + * The container descriptor supports only a single group per container.
> >> + * Required by the API as the container is not supplied with the IOMMU group
> >> + * at the moment of initialization.
> >> + */
> >> +struct tce_container {
> >> + struct mutex lock;
> >> + struct iommu_table *tbl;
> >> +};
> >> +
> >> +static void *tce_iommu_open(unsigned long arg)
> >> +{
> >> + struct tce_container *container;
> >> +
> >> + if (arg != VFIO_SPAPR_TCE_IOMMU) {
> >> + printk(KERN_ERR "tce_vfio: Wrong IOMMU type\n");
> >> + return ERR_PTR(-EINVAL);
> >> + }
> >> +
> >> + container = kzalloc(sizeof(*container), GFP_KERNEL);
> >> + if (!container)
> >> + return ERR_PTR(-ENOMEM);
> >> +
> >> + mutex_init(&container->lock);
> >> +
> >> + return container;
> >> +}
> >> +
> >> +static void tce_iommu_release(void *iommu_data)
> >> +{
> >> + struct tce_container *container = iommu_data;
> >> +
> >> + WARN_ON(container->tbl && !container->tbl->it_group);
> >
> > I think your patch ordering is backwards here. it_group isn't added
> > until 2/2. I'd really like to see the arch/powerpc code approved and
> > merged by the powerpc maintainer before we add the code that makes use
> > of it into vfio. Otherwise we just get lots of churn if interfaces
> > change or they disapprove of it altogether.
>
>
> Makes sense, thanks.
>
>
> >> + if (container->tbl && container->tbl->it_group)
> >> + tce_iommu_detach_group(iommu_data, container->tbl->it_group);
> >> +
> >> + mutex_destroy(&container->lock);
> >> +
> >> + kfree(container);
> >> +}
> >> +
> >> +static long tce_iommu_ioctl(void *iommu_data,
> >> + unsigned int cmd, unsigned long arg)
> >> +{
> >> + struct tce_container *container = iommu_data;
> >> + unsigned long minsz;
> >> +
> >> + switch (cmd) {
> >> + case VFIO_CHECK_EXTENSION: {
> >> + return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
> >> + }
> >> + case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
> >> + struct vfio_iommu_spapr_tce_info info;
> >> + struct iommu_table *tbl = container->tbl;
> >> +
> >> + if (WARN_ON(!tbl))
> >> + return -ENXIO;
> >> +
> >> + minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
> >> + dma64_window_size);
> >> +
> >> + if (copy_from_user(&info, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + if (info.argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
> >> + info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
> >> + info.dma64_window_start = 0;
> >> + info.dma64_window_size = 0;
> >> + info.flags = 0;
> >> +
> >> + if (copy_to_user((void __user *)arg, &info, minsz))
> >> + return -EFAULT;
> >> +
> >> + return 0;
> >> + }
> >> + case VFIO_IOMMU_MAP_DMA: {
> >> + vfio_iommu_spapr_tce_dma_map param;
> >> + struct iommu_table *tbl = container->tbl;
> >> + enum dma_data_direction direction = DMA_NONE;
> >> +
> >> + if (WARN_ON(!tbl))
> >> + return -ENXIO;
> >> +
> >> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
> >> +
> >> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + if (param.argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + if ((param.flags & VFIO_DMA_MAP_FLAG_READ) &&
> >> + (param.flags & VFIO_DMA_MAP_FLAG_WRITE)) {
> >> + direction = DMA_BIDIRECTIONAL;
> >> + } else if (param.flags & VFIO_DMA_MAP_FLAG_READ) {
> >> + direction = DMA_TO_DEVICE;
> >> + } else if (param.flags & VFIO_DMA_MAP_FLAG_WRITE) {
> >> + direction = DMA_FROM_DEVICE;
> >> + }
> >> +
> >> + param.size += param.iova & ~IOMMU_PAGE_MASK;
> >> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
> >
> > On x86 we force iova, vaddr, and size to all be aligned to the smallest
> > page granularity of the iommu and return -EINVAL if it doesn't fit.
> > What does it imply to the user if they're always aligned to work here?
> > Won't this interface happily map overlapping entries with no indication
> > to the user that the previous mapping is no longer valid?
> > Maybe another reason why a combined unmap/map makes me nervous, we have
> > to assume the user knows what they're doing.
>
>
> I got used to guests which do know what they are doing so I am pretty calm :)
> but ok, I'll move alignment to the QEMU, it makes sense.
>
>
> >> +
> >> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
> >> + param.vaddr & IOMMU_PAGE_MASK, direction,
> >> + param.size >> IOMMU_PAGE_SHIFT);
> >> + }
> >> + case VFIO_IOMMU_UNMAP_DMA: {
> >> + vfio_iommu_spapr_tce_dma_unmap param;
> >> + struct iommu_table *tbl = container->tbl;
> >> +
> >> + if (WARN_ON(!tbl))
> >> + return -ENXIO;
> >> +
> >> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
> >> +
> >> + if (copy_from_user(¶m, (void __user *)arg, minsz))
> >> + return -EFAULT;
> >> +
> >> + if (param.argsz < minsz)
> >> + return -EINVAL;
> >> +
> >> + param.size += param.iova & ~IOMMU_PAGE_MASK;
> >> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
> >> +
> >> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
> >> + 0, DMA_NONE, param.size >> IOMMU_PAGE_SHIFT);
> >> + }
> >> + default:
> >> + printk(KERN_WARNING "tce_vfio: unexpected cmd %x\n", cmd);
> >
> > pr_warn
> >
> >> + }
> >> +
> >> + return -ENOTTY;
> >> +}
> >> +
> >> +static int tce_iommu_attach_group(void *iommu_data,
> >> + struct iommu_group *iommu_group)
> >> +{
> >> + struct tce_container *container = iommu_data;
> >> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
> >> +
> >> + BUG_ON(!tbl);
> >> + mutex_lock(&container->lock);
> >> + pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
> >> + iommu_group_id(iommu_group), iommu_group);
> >> + if (container->tbl) {
> >> + printk(KERN_WARNING "tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
> >
> > pr_warn
> >
> >> + iommu_group_id(container->tbl->it_group),
> >> + iommu_group_id(iommu_group));
> >> + mutex_unlock(&container->lock);
> >> + return -EBUSY;
> >> + }
> >> +
> >> + container->tbl = tbl;
> >
> > Would it be too much paranoia to clear all the tce here as you do below
> > on detach?
>
> Guess so. I do unmap on detach() and the guest calls put_tce(0) (i.e.
> unmaps) the whole DMA window at the boot time.
But that's just one user of this interface, we can't assume they'll all
be so agreeable. If any tces were enabled here, a malicious user would
have a window to host memory, right? Thanks,
Alex
^ permalink raw reply
* Re: [PATCH] vfio powerpc: enabled and supported on powernv platform
From: Alex Williamson @ 2012-11-27 4:23 UTC (permalink / raw)
To: Alexey Kardashevskiy
Cc: kvm@vger.kernel.org, linux-kernel@vger.kernel.org, Paul Mackerras,
Sethi Varun-B16395, linuxppc-dev@lists.ozlabs.org, David Gibson
In-Reply-To: <50B4334E.2010802@ozlabs.ru>
On Tue, 2012-11-27 at 14:28 +1100, Alexey Kardashevskiy wrote:
> On 27/11/12 05:04, Alex Williamson wrote:
> > On Mon, 2012-11-26 at 08:18 -0700, Alex Williamson wrote:
> >> On Fri, 2012-11-23 at 13:02 +1100, Alexey Kardashevskiy wrote:
> >>> On 22/11/12 22:56, Sethi Varun-B16395 wrote:
> >>>>
> >>>>
> >>>>> -----Original Message-----
> >>>>> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> >>>>> owner@vger.kernel.org] On Behalf Of Alex Williamson
> >>>>> Sent: Tuesday, November 20, 2012 11:50 PM
> >>>>> To: Alexey Kardashevskiy
> >>>>> Cc: Benjamin Herrenschmidt; Paul Mackerras; linuxppc-
> >>>>> dev@lists.ozlabs.org; linux-kernel@vger.kernel.org; kvm@vger.kernel.org;
> >>>>> David Gibson
> >>>>> Subject: Re: [PATCH] vfio powerpc: enabled and supported on powernv
> >>>>> platform
> >>>>>
> >>>>> On Tue, 2012-11-20 at 11:48 +1100, Alexey Kardashevskiy wrote:
> >>>>>> VFIO implements platform independent stuff such as a PCI driver, BAR
> >>>>>> access (via read/write on a file descriptor or direct mapping when
> >>>>>> possible) and IRQ signaling.
> >>>>>> The platform dependent part includes IOMMU initialization and
> >>>>>> handling.
> >>>>>>
> >>>>>> This patch initializes IOMMU groups based on the IOMMU configuration
> >>>>>> discovered during the PCI scan, only POWERNV platform is supported at
> >>>>>> the moment.
> >>>>>>
> >>>>>> Also the patch implements an VFIO-IOMMU driver which manages DMA
> >>>>>> mapping/unmapping requests coming from the client (now QEMU). It also
> >>>>>> returns a DMA window information to let the guest initialize the
> >>>>>> device tree for a guest OS properly. Although this driver has been
> >>>>>> tested only on POWERNV, it should work on any platform supporting TCE
> >>>>>> tables.
> >>>>>>
> >>>>>> To enable VFIO on POWER, enable SPAPR_TCE_IOMMU config option.
> >>>>>>
> >>>>>> Cc: David Gibson <david@gibson.dropbear.id.au>
> >>>>>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
> >>>>>> ---
> >>>>>> arch/powerpc/include/asm/iommu.h | 6 +
> >>>>>> arch/powerpc/kernel/iommu.c | 140 +++++++++++++++++++
> >>>>>> arch/powerpc/platforms/powernv/pci.c | 135 +++++++++++++++++++
> >>>>>> drivers/iommu/Kconfig | 8 ++
> >>>>>> drivers/vfio/Kconfig | 6 +
> >>>>>> drivers/vfio/Makefile | 1 +
> >>>>>> drivers/vfio/vfio_iommu_spapr_tce.c | 247
> >>>>> ++++++++++++++++++++++++++++++++++
> >>>>>> include/linux/vfio.h | 20 +++
> >>>>>> 8 files changed, 563 insertions(+)
> >>>>>> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
> >>>>>>
> >>>>>> diff --git a/arch/powerpc/include/asm/iommu.h
> >>>>>> b/arch/powerpc/include/asm/iommu.h
> >>>>>> index cbfe678..5ba66cb 100644
> >>>>>> --- a/arch/powerpc/include/asm/iommu.h
> >>>>>> +++ b/arch/powerpc/include/asm/iommu.h
> >>>>>> @@ -64,30 +64,33 @@ struct iommu_pool { }
> >>>>>> ____cacheline_aligned_in_smp;
> >>>>>>
> >>>>>> struct iommu_table {
> >>>>>> unsigned long it_busno; /* Bus number this table belongs to */
> >>>>>> unsigned long it_size; /* Size of iommu table in entries */
> >>>>>> unsigned long it_offset; /* Offset into global table */
> >>>>>> unsigned long it_base; /* mapped address of tce table */
> >>>>>> unsigned long it_index; /* which iommu table this is */
> >>>>>> unsigned long it_type; /* type: PCI or Virtual Bus */
> >>>>>> unsigned long it_blocksize; /* Entries in each block (cacheline)
> >>>>> */
> >>>>>> unsigned long poolsize;
> >>>>>> unsigned long nr_pools;
> >>>>>> struct iommu_pool large_pool;
> >>>>>> struct iommu_pool pools[IOMMU_NR_POOLS];
> >>>>>> unsigned long *it_map; /* A simple allocation bitmap for now
> >>>>> */
> >>>>>> +#ifdef CONFIG_IOMMU_API
> >>>>>> + struct iommu_group *it_group;
> >>>>>> +#endif
> >>>>>> };
> >>>>>>
> >>>>>> struct scatterlist;
> >>>>>>
> >>>>>> static inline void set_iommu_table_base(struct device *dev, void
> >>>>>> *base) {
> >>>>>> dev->archdata.dma_data.iommu_table_base = base; }
> >>>>>>
> >>>>>> static inline void *get_iommu_table_base(struct device *dev) {
> >>>>>> return dev->archdata.dma_data.iommu_table_base;
> >>>>>> }
> >>>>>>
> >>>>>> /* Frees table for an individual device node */ @@ -135,17 +138,20 @@
> >>>>>> static inline void pci_iommu_init(void) { } extern void
> >>>>>> alloc_dart_table(void); #if defined(CONFIG_PPC64) &&
> >>>>>> defined(CONFIG_PM) static inline void iommu_save(void) {
> >>>>>> if (ppc_md.iommu_save)
> >>>>>> ppc_md.iommu_save();
> >>>>>> }
> >>>>>>
> >>>>>> static inline void iommu_restore(void) {
> >>>>>> if (ppc_md.iommu_restore)
> >>>>>> ppc_md.iommu_restore();
> >>>>>> }
> >>>>>> #endif
> >>>>>>
> >>>>>> +extern long iommu_put_tces(struct iommu_table *tbl, unsigned long
> >>>>> entry, uint64_t tce,
> >>>>>> + enum dma_data_direction direction, unsigned long pages);
> >>>>>> +
> >>>>>> #endif /* __KERNEL__ */
> >>>>>> #endif /* _ASM_IOMMU_H */
> >>>>>> diff --git a/arch/powerpc/kernel/iommu.c b/arch/powerpc/kernel/iommu.c
> >>>>>> index ff5a6ce..94f614b 100644
> >>>>>> --- a/arch/powerpc/kernel/iommu.c
> >>>>>> +++ b/arch/powerpc/kernel/iommu.c
> >>>>>> @@ -32,30 +32,31 @@
> >>>>>> #include <linux/dma-mapping.h>
> >>>>>> #include <linux/bitmap.h>
> >>>>>> #include <linux/iommu-helper.h>
> >>>>>> #include <linux/crash_dump.h>
> >>>>>> #include <linux/hash.h>
> >>>>>> #include <linux/fault-inject.h>
> >>>>>> #include <linux/pci.h>
> >>>>>> #include <asm/io.h>
> >>>>>> #include <asm/prom.h>
> >>>>>> #include <asm/iommu.h>
> >>>>>> #include <asm/pci-bridge.h>
> >>>>>> #include <asm/machdep.h>
> >>>>>> #include <asm/kdump.h>
> >>>>>> #include <asm/fadump.h>
> >>>>>> #include <asm/vio.h>
> >>>>>> +#include <asm/tce.h>
> >>>>>>
> >>>>>> #define DBG(...)
> >>>>>>
> >>>>>> static int novmerge;
> >>>>>>
> >>>>>> static void __iommu_free(struct iommu_table *, dma_addr_t, unsigned
> >>>>>> int);
> >>>>>>
> >>>>>> static int __init setup_iommu(char *str) {
> >>>>>> if (!strcmp(str, "novmerge"))
> >>>>>> novmerge = 1;
> >>>>>> else if (!strcmp(str, "vmerge"))
> >>>>>> novmerge = 0;
> >>>>>> return 1;
> >>>>>> }
> >>>>>> @@ -844,15 +845,154 @@ void *iommu_alloc_coherent(struct device *dev,
> >>>>>> struct iommu_table *tbl, }
> >>>>>>
> >>>>>> void iommu_free_coherent(struct iommu_table *tbl, size_t size,
> >>>>>> void *vaddr, dma_addr_t dma_handle) {
> >>>>>> if (tbl) {
> >>>>>> unsigned int nio_pages;
> >>>>>>
> >>>>>> size = PAGE_ALIGN(size);
> >>>>>> nio_pages = size >> IOMMU_PAGE_SHIFT;
> >>>>>> iommu_free(tbl, dma_handle, nio_pages);
> >>>>>> size = PAGE_ALIGN(size);
> >>>>>> free_pages((unsigned long)vaddr, get_order(size));
> >>>>>> }
> >>>>>> }
> >>>>>> +
> >>>>>> +#ifdef CONFIG_IOMMU_API
> >>>>>> +/*
> >>>>>> + * SPAPR TCE API
> >>>>>> + */
> >>>>>> +static struct page *free_tce(struct iommu_table *tbl, unsigned long
> >>>>>> +entry) {
> >>>>>> + struct page *page = NULL;
> >>>>>
> >>>>> NULL initialization doesn't appear to be necessary
> >>>>>
> >>>>>> + unsigned long oldtce;
> >>>>>> +
> >>>>>> + oldtce = ppc_md.tce_get(tbl, entry);
> >>>>>> +
> >>>>>> + if (!(oldtce & (TCE_PCI_WRITE | TCE_PCI_READ)))
> >>>>>> + return NULL;
> >>>>>> +
> >>>>>> + page = pfn_to_page(oldtce >> PAGE_SHIFT);
> >>>>>> +
> >>>>>> + WARN_ON(!page);
> >>>>>> + if (page && (oldtce & TCE_PCI_WRITE))
> >>>>>> + SetPageDirty(page);
> >>>>>> + ppc_md.tce_free(tbl, entry, 1);
> >>>>>> +
> >>>>>> + return page;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static int put_tce(struct iommu_table *tbl, unsigned long entry,
> >>>>>> + uint64_t tce, enum dma_data_direction direction) {
> >>>>>> + int ret;
> >>>>>> + struct page *page = NULL;
> >>>>>> + unsigned long kva, offset;
> >>>>>> +
> >>>>>> + /* Map new TCE */
> >>>>>> + offset = (tce & IOMMU_PAGE_MASK) - (tce & PAGE_MASK);
> >>>>>> + ret = get_user_pages_fast(tce & PAGE_MASK, 1,
> >>>>>> + direction != DMA_TO_DEVICE, &page);
> >>>>>> + if (ret < 1) {
> >>>>>> + printk(KERN_ERR "tce_vfio: get_user_pages_fast failed
> >>>>> tce=%llx ioba=%lx ret=%d\n",
> >>>>>> + tce, entry << IOMMU_PAGE_SHIFT, ret);
> >>>>>> + if (!ret)
> >>>>>> + ret = -EFAULT;
> >>>>>
> >>>>> Missing return ret? Otherwise we've got some bogus uses of page below
> >>>>> and we're setting ret for no reason here.
> >>>>>
> >>>>>> + }
> >>>>>> +
> >>>>>> + kva = (unsigned long) page_address(page);
> >>>>>> + kva += offset;
> >>>>>> +
> >>>>>> + /* tce_build receives a virtual address */
> >>>>>> + entry += tbl->it_offset; /* Offset into real TCE table */
> >>>>>> + ret = ppc_md.tce_build(tbl, entry, 1, kva, direction, NULL);
> >>>>>> +
> >>>>>> + /* tce_build() only returns non-zero for transient errors */
> >>>>>> + if (unlikely(ret)) {
> >>>>>> + printk(KERN_ERR "tce_vfio: tce_put failed on tce=%llx
> >>>>> ioba=%lx kva=%lx ret=%d\n",
> >>>>>> + tce, entry << IOMMU_PAGE_SHIFT, kva, ret);
> >>>>>> + put_page(page);
> >>>>>> + return -EIO;
> >>>>>> + }
> >>>>>> +
> >>>>>> + return 0;
> >>>>>> +}
> >>>>>> +
> >>>>>> +static void tce_flush(struct iommu_table *tbl) {
> >>>>>> + /* Flush/invalidate TLB caches if necessary */
> >>>>>> + if (ppc_md.tce_flush)
> >>>>>> + ppc_md.tce_flush(tbl);
> >>>>>> +
> >>>>>> + /* Make sure updates are seen by hardware */
> >>>>>> + mb();
> >>>>>> +}
> >>>>>> +
> >>>>>> +long iommu_put_tces(struct iommu_table *tbl, unsigned long entry,
> >>>>> uint64_t tce,
> >>>>>> + enum dma_data_direction direction, unsigned long pages) {
> >>>>>> + int i, ret = 0, pages_to_put = 0;
> >>>>>> + struct page *page;
> >>>>>> + struct iommu_pool *pool = get_pool(tbl, entry);
> >>>>>> + struct page **oldpages;
> >>>>>> + const int oldpagesnum = PAGE_SIZE/sizeof(*oldpages);
> >>>>>> +
> >>>>>> + BUILD_BUG_ON(PAGE_SIZE < IOMMU_PAGE_SIZE);
> >>>>>> +
> >>>>>> + /* Handle a single page request without allocation
> >>>>>> + of pages-to-release array */
> >>>>>> + if (pages == 1) {
> >>>>>> + spin_lock(&(pool->lock));
> >>>>>> + page = free_tce(tbl, entry);
> >>>>>> +
> >>>>>> + if (direction != DMA_NONE)
> >>>>>> + ret = put_tce(tbl, entry, tce, direction);
> >>>>>> +
> >>>>>> + tce_flush(tbl);
> >>>>>> +
> >>>>>> + if (page)
> >>>>>> + put_page(page);
> >>>>>> +
> >>>>>> + spin_unlock(&(pool->lock));
> >>>>>> + return ret;
> >>>>>> + }
> >>>>>> +
> >>>>>> + /* Releasing multiple pages */
> >>>>>> + /* Allocate an array for pages to be released after TCE table
> >>>>>> + is updated */
> >>>>>> + oldpages = kmalloc(PAGE_SIZE, GFP_KERNEL);
> >>>>>> + if (!oldpages)
> >>>>>> + return -ENOMEM;
> >>>>>> +
> >>>>>> + spin_lock(&(pool->lock));
> >>>>>> +
> >>>>>> + for (i = 0; (i < pages) && !ret; ++i, ++entry, tce +=
> >>>>> IOMMU_PAGE_SIZE) {
> >>>>>> + page = free_tce(tbl, entry);
> >>>>>> + if (page) {
> >>>>>> + oldpages[pages_to_put] = page;
> >>>>>> + ++pages_to_put;
> >>>>>> + }
> >>>>>> +
> >>>>>> + if (direction != DMA_NONE)
> >>>>>> + ret = put_tce(tbl, entry, tce, direction);
> >>>>>> +
> >>>>>> + /* Release old pages if we reached the end of oldpages[] or
> >>>>>> + it is the last page or we are about to exit the loop */
> >>>>>> + if ((pages_to_put == oldpagesnum) || (i == pages - 1) || ret)
> >>>>> {
> >>>>>> + tce_flush(tbl);
> >>>>>
> >>>>> Avoiding tce_flush() is the reason for all this extra overhead, right?
> >>>>> I wonder if it'd be cleaner separating map vs unmap, where the map case
> >>>>> can avoid the oldpages array... but that means inserting new mappings on
> >>>>> top of old ones wouldn't put the pages.
> >>>
> >>>
> >>> Yes, we do not want to loose pages if the guest forgot to unmap them.
> >>
> >> Hmm, does that mean we're not actively clearing tce entries or somehow
> >> disabling the iommu window when the iommu is released through vfio?
> >
> > Ok, I see tces are put on shutdown via tce_iommu_detach_group, so you're
> > more concerned about the guest simply mapping over top of it's own
> > mappings. Is that common? Is it common enough for every multi-page
> > mapping to assume it will happen? I know this is a performance
> > sensitive path for you and it seems like a map-only w/ fallback to
> > unmap, remap would be better in the general case.
>
>
> I do not get it. Where exactly does the performance suffer?
> iommu_put_tces() with non zero "tce" (i.e. "map") has to check if the entry
> is not used, at least to return EBUSY when it is, and this check is
> performed. If it is zero, there is no overhead at all. And it is going to
> be the 99.(9)% case as the guest (un)maps one page per call.
I was mostly concerned about the kmalloc in your mapping path. If you
had a map-only path it could scan the whole range to verify it's clear
and return EBUSY w/o allocating a buffer. A second pass could do the
actual mappings. Maybe it's not much of a win if you expect 99% of the
mappings to be single pages but since you effectively have a pv iommu
interface I wouldn't be surprised if they get batched in the guest.
> Generally speaking we want to move "put tce" completely to the kernel for
> the (much) better performance and vfio won't be dealing with it all.
Right, but let's not use that as an excuse to be lazy and not ask if we
can do better here.
> We already agreed that SPAPR TCE driver uses x86 (aka type1) API but I do
> not see why the powerpc implementation should look x86 alike as it still
> operates with powerpc machine dependent callbacks so the reader has to have
> some powerpc knowledge.
I'm only using x86 as an example because it's the only one we have. I
don't think anything we're talking about here is x86-ish or powerpc-ish.
There's a kmalloc in a performance path and I'm asking if we can get rid
of it. I'm also nervous that we're silently doing fixups on user
parameters to adjust mapping sizes and clear overlaps without any
warning to the user. Thanks,
Alex
^ permalink raw reply
* Re: [PATCH 1/2] vfio powerpc: implemented IOMMU driver for VFIO
From: Alexey Kardashevskiy @ 2012-11-27 4:06 UTC (permalink / raw)
To: Alex Williamson
Cc: kvm, linux-kernel, Paul Mackerras, linuxppc-dev, David Gibson
In-Reply-To: <1353954038.1809.114.camel@bling.home>
On 27/11/12 05:20, Alex Williamson wrote:
> On Fri, 2012-11-23 at 20:03 +1100, Alexey Kardashevskiy wrote:
>> VFIO implements platform independent stuff such as
>> a PCI driver, BAR access (via read/write on a file descriptor
>> or direct mapping when possible) and IRQ signaling.
>>
>> The platform dependent part includes IOMMU initialization
>> and handling. This patch implements an IOMMU driver for VFIO
>> which does mapping/unmapping pages for the guest IO and
>> provides information about DMA window (required by a POWERPC
>> guest).
>>
>> The counterpart in QEMU is required to support this functionality.
>>
>> Cc: David Gibson <david@gibson.dropbear.id.au>
>> Signed-off-by: Alexey Kardashevskiy <aik@ozlabs.ru>
>> ---
>> drivers/vfio/Kconfig | 6 +
>> drivers/vfio/Makefile | 1 +
>> drivers/vfio/vfio_iommu_spapr_tce.c | 247 +++++++++++++++++++++++++++++++++++
>> include/linux/vfio.h | 20 +++
>> 4 files changed, 274 insertions(+)
>> create mode 100644 drivers/vfio/vfio_iommu_spapr_tce.c
>>
>> diff --git a/drivers/vfio/Kconfig b/drivers/vfio/Kconfig
>> index 7cd5dec..b464687 100644
>> --- a/drivers/vfio/Kconfig
>> +++ b/drivers/vfio/Kconfig
>> @@ -3,10 +3,16 @@ config VFIO_IOMMU_TYPE1
>> depends on VFIO
>> default n
>>
>> +config VFIO_IOMMU_SPAPR_TCE
>> + tristate
>> + depends on VFIO && SPAPR_TCE_IOMMU
>> + default n
>> +
>> menuconfig VFIO
>> tristate "VFIO Non-Privileged userspace driver framework"
>> depends on IOMMU_API
>> select VFIO_IOMMU_TYPE1 if X86
>> + select VFIO_IOMMU_SPAPR_TCE if PPC_POWERNV
>> help
>> VFIO provides a framework for secure userspace device drivers.
>> See Documentation/vfio.txt for more details.
>> diff --git a/drivers/vfio/Makefile b/drivers/vfio/Makefile
>> index 2398d4a..72bfabc 100644
>> --- a/drivers/vfio/Makefile
>> +++ b/drivers/vfio/Makefile
>> @@ -1,3 +1,4 @@
>> obj-$(CONFIG_VFIO) += vfio.o
>> obj-$(CONFIG_VFIO_IOMMU_TYPE1) += vfio_iommu_type1.o
>> +obj-$(CONFIG_VFIO_IOMMU_SPAPR_TCE) += vfio_iommu_spapr_tce.o
>> obj-$(CONFIG_VFIO_PCI) += pci/
>> diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c
>> new file mode 100644
>> index 0000000..46a6298
>> --- /dev/null
>> +++ b/drivers/vfio/vfio_iommu_spapr_tce.c
>> @@ -0,0 +1,247 @@
>> +/*
>> + * VFIO: IOMMU DMA mapping support for TCE on POWER
>> + *
>> + * Copyright (C) 2012 IBM Corp. All rights reserved.
>> + * Author: Alexey Kardashevskiy <aik@ozlabs.ru>
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License version 2 as
>> + * published by the Free Software Foundation.
>> + *
>> + * Derived from original vfio_iommu_type1.c:
>> + * Copyright (C) 2012 Red Hat, Inc. All rights reserved.
>> + * Author: Alex Williamson <alex.williamson@redhat.com>
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/slab.h>
>> +#include <linux/uaccess.h>
>> +#include <linux/err.h>
>> +#include <linux/vfio.h>
>> +#include <asm/iommu.h>
>> +
>> +#define DRIVER_VERSION "0.1"
>> +#define DRIVER_AUTHOR "aik@ozlabs.ru"
>> +#define DRIVER_DESC "VFIO IOMMU SPAPR TCE"
>> +
>> +static void tce_iommu_detach_group(void *iommu_data,
>> + struct iommu_group *iommu_group);
>> +
>> +/*
>> + * VFIO IOMMU fd for SPAPR_TCE IOMMU implementation
>> + */
>> +
>> +/*
>> + * The container descriptor supports only a single group per container.
>> + * Required by the API as the container is not supplied with the IOMMU group
>> + * at the moment of initialization.
>> + */
>> +struct tce_container {
>> + struct mutex lock;
>> + struct iommu_table *tbl;
>> +};
>> +
>> +static void *tce_iommu_open(unsigned long arg)
>> +{
>> + struct tce_container *container;
>> +
>> + if (arg != VFIO_SPAPR_TCE_IOMMU) {
>> + printk(KERN_ERR "tce_vfio: Wrong IOMMU type\n");
>> + return ERR_PTR(-EINVAL);
>> + }
>> +
>> + container = kzalloc(sizeof(*container), GFP_KERNEL);
>> + if (!container)
>> + return ERR_PTR(-ENOMEM);
>> +
>> + mutex_init(&container->lock);
>> +
>> + return container;
>> +}
>> +
>> +static void tce_iommu_release(void *iommu_data)
>> +{
>> + struct tce_container *container = iommu_data;
>> +
>> + WARN_ON(container->tbl && !container->tbl->it_group);
>
> I think your patch ordering is backwards here. it_group isn't added
> until 2/2. I'd really like to see the arch/powerpc code approved and
> merged by the powerpc maintainer before we add the code that makes use
> of it into vfio. Otherwise we just get lots of churn if interfaces
> change or they disapprove of it altogether.
Makes sense, thanks.
>> + if (container->tbl && container->tbl->it_group)
>> + tce_iommu_detach_group(iommu_data, container->tbl->it_group);
>> +
>> + mutex_destroy(&container->lock);
>> +
>> + kfree(container);
>> +}
>> +
>> +static long tce_iommu_ioctl(void *iommu_data,
>> + unsigned int cmd, unsigned long arg)
>> +{
>> + struct tce_container *container = iommu_data;
>> + unsigned long minsz;
>> +
>> + switch (cmd) {
>> + case VFIO_CHECK_EXTENSION: {
>> + return (arg == VFIO_SPAPR_TCE_IOMMU) ? 1 : 0;
>> + }
>> + case VFIO_IOMMU_SPAPR_TCE_GET_INFO: {
>> + struct vfio_iommu_spapr_tce_info info;
>> + struct iommu_table *tbl = container->tbl;
>> +
>> + if (WARN_ON(!tbl))
>> + return -ENXIO;
>> +
>> + minsz = offsetofend(struct vfio_iommu_spapr_tce_info,
>> + dma64_window_size);
>> +
>> + if (copy_from_user(&info, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (info.argsz < minsz)
>> + return -EINVAL;
>> +
>> + info.dma32_window_start = tbl->it_offset << IOMMU_PAGE_SHIFT;
>> + info.dma32_window_size = tbl->it_size << IOMMU_PAGE_SHIFT;
>> + info.dma64_window_start = 0;
>> + info.dma64_window_size = 0;
>> + info.flags = 0;
>> +
>> + if (copy_to_user((void __user *)arg, &info, minsz))
>> + return -EFAULT;
>> +
>> + return 0;
>> + }
>> + case VFIO_IOMMU_MAP_DMA: {
>> + vfio_iommu_spapr_tce_dma_map param;
>> + struct iommu_table *tbl = container->tbl;
>> + enum dma_data_direction direction = DMA_NONE;
>> +
>> + if (WARN_ON(!tbl))
>> + return -ENXIO;
>> +
>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_map, size);
>> +
>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (param.argsz < minsz)
>> + return -EINVAL;
>> +
>> + if ((param.flags & VFIO_DMA_MAP_FLAG_READ) &&
>> + (param.flags & VFIO_DMA_MAP_FLAG_WRITE)) {
>> + direction = DMA_BIDIRECTIONAL;
>> + } else if (param.flags & VFIO_DMA_MAP_FLAG_READ) {
>> + direction = DMA_TO_DEVICE;
>> + } else if (param.flags & VFIO_DMA_MAP_FLAG_WRITE) {
>> + direction = DMA_FROM_DEVICE;
>> + }
>> +
>> + param.size += param.iova & ~IOMMU_PAGE_MASK;
>> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
>
> On x86 we force iova, vaddr, and size to all be aligned to the smallest
> page granularity of the iommu and return -EINVAL if it doesn't fit.
> What does it imply to the user if they're always aligned to work here?
> Won't this interface happily map overlapping entries with no indication
> to the user that the previous mapping is no longer valid?
> Maybe another reason why a combined unmap/map makes me nervous, we have
> to assume the user knows what they're doing.
I got used to guests which do know what they are doing so I am pretty calm :)
but ok, I'll move alignment to the QEMU, it makes sense.
>> +
>> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
>> + param.vaddr & IOMMU_PAGE_MASK, direction,
>> + param.size >> IOMMU_PAGE_SHIFT);
>> + }
>> + case VFIO_IOMMU_UNMAP_DMA: {
>> + vfio_iommu_spapr_tce_dma_unmap param;
>> + struct iommu_table *tbl = container->tbl;
>> +
>> + if (WARN_ON(!tbl))
>> + return -ENXIO;
>> +
>> + minsz = offsetofend(vfio_iommu_spapr_tce_dma_unmap, size);
>> +
>> + if (copy_from_user(¶m, (void __user *)arg, minsz))
>> + return -EFAULT;
>> +
>> + if (param.argsz < minsz)
>> + return -EINVAL;
>> +
>> + param.size += param.iova & ~IOMMU_PAGE_MASK;
>> + param.size = _ALIGN_UP(param.size, IOMMU_PAGE_SIZE);
>> +
>> + return iommu_put_tces(tbl, param.iova >> IOMMU_PAGE_SHIFT,
>> + 0, DMA_NONE, param.size >> IOMMU_PAGE_SHIFT);
>> + }
>> + default:
>> + printk(KERN_WARNING "tce_vfio: unexpected cmd %x\n", cmd);
>
> pr_warn
>
>> + }
>> +
>> + return -ENOTTY;
>> +}
>> +
>> +static int tce_iommu_attach_group(void *iommu_data,
>> + struct iommu_group *iommu_group)
>> +{
>> + struct tce_container *container = iommu_data;
>> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
>> +
>> + BUG_ON(!tbl);
>> + mutex_lock(&container->lock);
>> + pr_debug("tce_vfio: Attaching group #%u to iommu %p\n",
>> + iommu_group_id(iommu_group), iommu_group);
>> + if (container->tbl) {
>> + printk(KERN_WARNING "tce_vfio: Only one group per IOMMU container is allowed, existing id=%d, attaching id=%d\n",
>
> pr_warn
>
>> + iommu_group_id(container->tbl->it_group),
>> + iommu_group_id(iommu_group));
>> + mutex_unlock(&container->lock);
>> + return -EBUSY;
>> + }
>> +
>> + container->tbl = tbl;
>
> Would it be too much paranoia to clear all the tce here as you do below
> on detach?
Guess so. I do unmap on detach() and the guest calls put_tce(0) (i.e.
unmaps) the whole DMA window at the boot time.
> ie. is there any risk that there's leftover programming?
> x86 allocates a new domain on open of the iommu, so we always start out
> clean.
>> + mutex_unlock(&container->lock);
>> +
>> + return 0;
>> +}
>> +
>> +static void tce_iommu_detach_group(void *iommu_data,
>> + struct iommu_group *iommu_group)
>> +{
>> + struct tce_container *container = iommu_data;
>> + struct iommu_table *tbl = iommu_group_get_iommudata(iommu_group);
>> +
>> + BUG_ON(!tbl);
>> + mutex_lock(&container->lock);
>> + if (tbl != container->tbl) {
>> + printk(KERN_WARNING "tce_vfio: detaching group #%u, expected group is #%u\n",
>
> pr_warn
>
>> + iommu_group_id(iommu_group),
>> + iommu_group_id(tbl->it_group));
>> + } else {
>> +
>> + pr_debug("tce_vfio: detaching group #%u from iommu %p\n",
>> + iommu_group_id(iommu_group), iommu_group);
>> +
>> + iommu_put_tces(tbl, tbl->it_offset, 0, DMA_NONE, tbl->it_size);
>
> So this cleans out any mappings when vfio is closed, good.
>
>> + container->tbl = NULL;
>> + }
>> + mutex_unlock(&container->lock);
>> +}
>> +
>> +const struct vfio_iommu_driver_ops tce_iommu_driver_ops = {
>> + .name = "iommu-vfio-powerpc",
>> + .owner = THIS_MODULE,
>> + .open = tce_iommu_open,
>> + .release = tce_iommu_release,
>> + .ioctl = tce_iommu_ioctl,
>> + .attach_group = tce_iommu_attach_group,
>> + .detach_group = tce_iommu_detach_group,
>> +};
>> +
>> +static int __init tce_iommu_init(void)
>> +{
>> + return vfio_register_iommu_driver(&tce_iommu_driver_ops);
>> +}
>> +
>> +static void __exit tce_iommu_cleanup(void)
>> +{
>> + vfio_unregister_iommu_driver(&tce_iommu_driver_ops);
>> +}
>> +
>> +module_init(tce_iommu_init);
>> +module_exit(tce_iommu_cleanup);
>> +
>> +MODULE_VERSION(DRIVER_VERSION);
>> +MODULE_LICENSE("GPL v2");
>> +MODULE_AUTHOR(DRIVER_AUTHOR);
>> +MODULE_DESCRIPTION(DRIVER_DESC);
>> +
>> diff --git a/include/linux/vfio.h b/include/linux/vfio.h
>> index 0a4f180..3ecd65c 100644
>> --- a/include/linux/vfio.h
>> +++ b/include/linux/vfio.h
>> @@ -99,6 +99,7 @@ extern void vfio_unregister_iommu_driver(
>> /* Extensions */
>>
>> #define VFIO_TYPE1_IOMMU 1
>> +#define VFIO_SPAPR_TCE_IOMMU 2
>>
>> /*
>> * The IOCTL interface is designed for extensibility by embedding the
>> @@ -442,4 +443,23 @@ struct vfio_iommu_type1_dma_unmap {
>>
>> #define VFIO_IOMMU_UNMAP_DMA _IO(VFIO_TYPE, VFIO_BASE + 14)
>>
>> +/* -------- Additional API for SPAPR TCE (Server POWERPC) IOMMU -------- */
>> +
>> +struct vfio_iommu_spapr_tce_info {
>> + __u32 argsz;
>> + __u32 flags;
>> + __u32 dma32_window_start;
>> + __u32 dma32_window_size;
>> + __u64 dma64_window_start;
>> + __u64 dma64_window_size;
>> +};
>
> Is there anything we can document about this?
I'll put some.
> It should probably list that size is in bytes. Is there any need to communicate the IOMMU page
> size here?
It is always 4k. I'll put it to comments.
>> +
>> +#define VFIO_IOMMU_SPAPR_TCE_GET_INFO _IO(VFIO_TYPE, VFIO_BASE + 12)
>> +
>> +/* Reuse type1 map/unmap structs as they are the same at the moment */
>> +typedef struct vfio_iommu_type1_dma_map vfio_iommu_spapr_tce_dma_map;
>> +typedef struct vfio_iommu_type1_dma_unmap vfio_iommu_spapr_tce_dma_unmap;
>> +
>> +/* ***************************************************************** */
>> +
>> #endif /* VFIO_H */
>
> Thanks,
>
> Alex
>
>
>
--
Alexey
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox