| Message ID | 1481298606-2550-1-git-send-email-ross.lagerwall@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/09/2016 10:50 AM, Ross Lagerwall wrote: > When relocating the p2m/initrd, take special care not to relocate it so > that is overlaps with the current location of the p2m/initrd. This is > needed since the full extent of the current location is not marked as a > reserved region in the e820 (and it shouldn't be since it is about to be > moved). > > This was seen to happen to a dom0 with a large initial p2m and a small > reserved region in the middle of the initial p2m. > > > /* > - * Find a free area in physical memory not yet reserved and compliant with > - * E820 map. > + * Find a free area in physical memory not yet reserved, compliant with the > + * E820 map and not overlapping with the pre-allocated area. > * Used to relocate pre-allocated areas like initrd or p2m list which are in > * conflict with the to be used E820 map. > * In case no area is found, return 0. Otherwise return the physical address > * of the area which is already reserved for convenience. > */ > -phys_addr_t __init xen_find_free_area(phys_addr_t size) > +phys_addr_t __init xen_find_free_area(phys_addr_t size, phys_addr_t cur_start, > + phys_addr_t cur_size) > { > unsigned mapcnt; > phys_addr_t addr, start; > @@ -652,7 +653,8 @@ phys_addr_t __init xen_find_free_area(phys_addr_t size) > continue; > start = entry->addr; > for (addr = start; addr < start + size; addr += PAGE_SIZE) { > - if (!memblock_is_reserved(addr)) > + if (!memblock_is_reserved(addr) && > + (addr < cur_start || addr >= cur_start + cur_size)) > continue; > start = addr + PAGE_SIZE; > if (start + size > entry->addr + entry->size) Wouldn't this in fact make it even more likely to return pointer to the currently allocated chunk? If we are in [cur_start, cur_start+cur_size) then we assign this address to 'start' and may well return it, no? -boris
On 12/09/2016 09:08 PM, Boris Ostrovsky wrote: > On 12/09/2016 10:50 AM, Ross Lagerwall wrote: >> When relocating the p2m/initrd, take special care not to relocate it so >> that is overlaps with the current location of the p2m/initrd. This is >> needed since the full extent of the current location is not marked as a >> reserved region in the e820 (and it shouldn't be since it is about to be >> moved). >> >> This was seen to happen to a dom0 with a large initial p2m and a small >> reserved region in the middle of the initial p2m. >> > > >> >> /* >> - * Find a free area in physical memory not yet reserved and compliant with >> - * E820 map. >> + * Find a free area in physical memory not yet reserved, compliant with the >> + * E820 map and not overlapping with the pre-allocated area. >> * Used to relocate pre-allocated areas like initrd or p2m list which are in >> * conflict with the to be used E820 map. >> * In case no area is found, return 0. Otherwise return the physical address >> * of the area which is already reserved for convenience. >> */ >> -phys_addr_t __init xen_find_free_area(phys_addr_t size) >> +phys_addr_t __init xen_find_free_area(phys_addr_t size, phys_addr_t cur_start, >> + phys_addr_t cur_size) >> { >> unsigned mapcnt; >> phys_addr_t addr, start; >> @@ -652,7 +653,8 @@ phys_addr_t __init xen_find_free_area(phys_addr_t size) >> continue; >> start = entry->addr; >> for (addr = start; addr < start + size; addr += PAGE_SIZE) { >> - if (!memblock_is_reserved(addr)) >> + if (!memblock_is_reserved(addr) && >> + (addr < cur_start || addr >= cur_start + cur_size)) >> continue; >> start = addr + PAGE_SIZE; >> if (start + size > entry->addr + entry->size) > > > Wouldn't this in fact make it even more likely to return pointer to the > currently allocated chunk? > > If we are in [cur_start, cur_start+cur_size) then we assign this address > to 'start' and may well return it, no? > If addr is in [cur_start, cur_start + cur_size) then start is set to addr + PAGE_SIZE and we go round the inner loop again. If the new addr is still in [cur_start, cur_start + cur_size) then we set start to addr + PAGE_SIZE and go round again... I can't see why it would return it It is treated the same as if addr is in a reserved memblock.
On 09/12/16 16:50, Ross Lagerwall wrote: > When relocating the p2m/initrd, take special care not to relocate it so > that is overlaps with the current location of the p2m/initrd. This is > needed since the full extent of the current location is not marked as a > reserved region in the e820 (and it shouldn't be since it is about to be > moved). > > This was seen to happen to a dom0 with a large initial p2m and a small > reserved region in the middle of the initial p2m. > > Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> Wouldn't it be more natural to memblock_reserve() the hypervisor supplied p2m list even in case of an E820 conflict and do a memblock_free() of that region after moving the p2m list in xen_relocate_p2m() ? This would avoid the need to supply a range to avoid for memory allocation when calling xen_find_free_area(), which is pointless in the initrd case (the original initrd area is already reserved via memblock_reserve() ). Juergen > --- > arch/x86/xen/mmu.c | 4 ++-- > arch/x86/xen/setup.c | 16 ++++++++++------ > arch/x86/xen/xen-ops.h | 5 +++-- > 3 files changed, 15 insertions(+), 10 deletions(-) > > diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c > index 7d5afdb..bc40325 100644 > --- a/arch/x86/xen/mmu.c > +++ b/arch/x86/xen/mmu.c > @@ -2074,7 +2074,7 @@ static phys_addr_t __init xen_early_virt_to_phys(unsigned long vaddr) > * Find a new area for the hypervisor supplied p2m list and relocate the p2m to > * this area. > */ > -void __init xen_relocate_p2m(void) > +void __init xen_relocate_p2m(phys_addr_t cur_start, phys_addr_t cur_size) > { > phys_addr_t size, new_area, pt_phys, pmd_phys, pud_phys; > unsigned long p2m_pfn, p2m_pfn_end, n_frames, pfn, pfn_end; > @@ -2092,7 +2092,7 @@ void __init xen_relocate_p2m(void) > n_pud = roundup(size, PGDIR_SIZE) >> PGDIR_SHIFT; > n_frames = n_pte + n_pt + n_pmd + n_pud; > > - new_area = xen_find_free_area(PFN_PHYS(n_frames)); > + new_area = xen_find_free_area(PFN_PHYS(n_frames), cur_start, cur_size); > if (!new_area) { > xen_raw_console_write("Can't find new memory area for p2m needed due to E820 map conflict\n"); > BUG(); > diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c > index f8960fc..513c48b 100644 > --- a/arch/x86/xen/setup.c > +++ b/arch/x86/xen/setup.c > @@ -634,14 +634,15 @@ bool __init xen_is_e820_reserved(phys_addr_t start, phys_addr_t size) > } > > /* > - * Find a free area in physical memory not yet reserved and compliant with > - * E820 map. > + * Find a free area in physical memory not yet reserved, compliant with the > + * E820 map and not overlapping with the pre-allocated area. > * Used to relocate pre-allocated areas like initrd or p2m list which are in > * conflict with the to be used E820 map. > * In case no area is found, return 0. Otherwise return the physical address > * of the area which is already reserved for convenience. > */ > -phys_addr_t __init xen_find_free_area(phys_addr_t size) > +phys_addr_t __init xen_find_free_area(phys_addr_t size, phys_addr_t cur_start, > + phys_addr_t cur_size) > { > unsigned mapcnt; > phys_addr_t addr, start; > @@ -652,7 +653,8 @@ phys_addr_t __init xen_find_free_area(phys_addr_t size) > continue; > start = entry->addr; > for (addr = start; addr < start + size; addr += PAGE_SIZE) { > - if (!memblock_is_reserved(addr)) > + if (!memblock_is_reserved(addr) && > + (addr < cur_start || addr >= cur_start + cur_size)) > continue; > start = addr + PAGE_SIZE; > if (start + size > entry->addr + entry->size) > @@ -726,7 +728,7 @@ static void __init xen_reserve_xen_mfnlist(void) > xen_raw_console_write("Xen hypervisor allocated p2m list conflicts with E820 map\n"); > BUG(); > #else > - xen_relocate_p2m(); > + xen_relocate_p2m(start, size); > #endif > } > > @@ -887,7 +889,9 @@ char * __init xen_memory_setup(void) > boot_params.hdr.ramdisk_size)) { > phys_addr_t new_area, start, size; > > - new_area = xen_find_free_area(boot_params.hdr.ramdisk_size); > + new_area = xen_find_free_area(boot_params.hdr.ramdisk_size, > + boot_params.hdr.ramdisk_image, > + boot_params.hdr.ramdisk_size); > if (!new_area) { > xen_raw_console_write("Can't find new memory area for initrd needed due to E820 map conflict\n"); > BUG(); > diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h > index 3cbce3b..d3342b8 100644 > --- a/arch/x86/xen/xen-ops.h > +++ b/arch/x86/xen/xen-ops.h > @@ -41,14 +41,15 @@ void __init xen_pt_check_e820(void); > void xen_mm_pin_all(void); > void xen_mm_unpin_all(void); > #ifdef CONFIG_X86_64 > -void __init xen_relocate_p2m(void); > +void __init xen_relocate_p2m(phys_addr_t cur_start, phys_addr_t cur_size); > #endif > > bool __init xen_is_e820_reserved(phys_addr_t start, phys_addr_t size); > unsigned long __ref xen_chk_extra_mem(unsigned long pfn); > void __init xen_inv_extra_mem(void); > void __init xen_remap_memory(void); > -phys_addr_t __init xen_find_free_area(phys_addr_t size); > +phys_addr_t __init xen_find_free_area(phys_addr_t size, phys_addr_t cur_start, > + phys_addr_t cur_size); > char * __init xen_memory_setup(void); > char * xen_auto_xlated_memory_setup(void); > void __init xen_arch_setup(void); >
On 12/12/2016 08:19 AM, Juergen Gross wrote: > On 09/12/16 16:50, Ross Lagerwall wrote: >> When relocating the p2m/initrd, take special care not to relocate it so >> that is overlaps with the current location of the p2m/initrd. This is >> needed since the full extent of the current location is not marked as a >> reserved region in the e820 (and it shouldn't be since it is about to be >> moved). >> >> This was seen to happen to a dom0 with a large initial p2m and a small >> reserved region in the middle of the initial p2m. >> >> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> > > Wouldn't it be more natural to memblock_reserve() the hypervisor > supplied p2m list even in case of an E820 conflict and do a > memblock_free() of that region after moving the p2m list in > xen_relocate_p2m() ? > > This would avoid the need to supply a range to avoid for memory > allocation when calling xen_find_free_area(), which is pointless in the > initrd case (the original initrd area is already reserved via > memblock_reserve() ). > I'm not familiar with the code, but I was concerned that if part of the hypervisor-supplied p2m was already reserved by memblock_reserve(), then the above approach would remove it when memblock_free is called. If this is not a valid concern, then I can send a patch to do it.
On 12/12/16 09:28, Ross Lagerwall wrote: > On 12/12/2016 08:19 AM, Juergen Gross wrote: >> On 09/12/16 16:50, Ross Lagerwall wrote: >>> When relocating the p2m/initrd, take special care not to relocate it so >>> that is overlaps with the current location of the p2m/initrd. This is >>> needed since the full extent of the current location is not marked as a >>> reserved region in the e820 (and it shouldn't be since it is about to be >>> moved). >>> >>> This was seen to happen to a dom0 with a large initial p2m and a small >>> reserved region in the middle of the initial p2m. >>> >>> Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> >> >> Wouldn't it be more natural to memblock_reserve() the hypervisor >> supplied p2m list even in case of an E820 conflict and do a >> memblock_free() of that region after moving the p2m list in >> xen_relocate_p2m() ? >> >> This would avoid the need to supply a range to avoid for memory >> allocation when calling xen_find_free_area(), which is pointless in the >> initrd case (the original initrd area is already reserved via >> memblock_reserve() ). >> > > I'm not familiar with the code, but I was concerned that if part of the > hypervisor-supplied p2m was already reserved by memblock_reserve(), then > the above approach would remove it when memblock_free is called. If this > is not a valid concern, then I can send a patch to do it. If this would be the case we'd be in trouble: after moving the p2m list to another location nobody should reference the old p2m list as it might be located at a position to be remapped due to E820 memory map constraints. Juergen
diff --git a/arch/x86/xen/mmu.c b/arch/x86/xen/mmu.c index 7d5afdb..bc40325 100644 --- a/arch/x86/xen/mmu.c +++ b/arch/x86/xen/mmu.c @@ -2074,7 +2074,7 @@ static phys_addr_t __init xen_early_virt_to_phys(unsigned long vaddr) * Find a new area for the hypervisor supplied p2m list and relocate the p2m to * this area. */ -void __init xen_relocate_p2m(void) +void __init xen_relocate_p2m(phys_addr_t cur_start, phys_addr_t cur_size) { phys_addr_t size, new_area, pt_phys, pmd_phys, pud_phys; unsigned long p2m_pfn, p2m_pfn_end, n_frames, pfn, pfn_end; @@ -2092,7 +2092,7 @@ void __init xen_relocate_p2m(void) n_pud = roundup(size, PGDIR_SIZE) >> PGDIR_SHIFT; n_frames = n_pte + n_pt + n_pmd + n_pud; - new_area = xen_find_free_area(PFN_PHYS(n_frames)); + new_area = xen_find_free_area(PFN_PHYS(n_frames), cur_start, cur_size); if (!new_area) { xen_raw_console_write("Can't find new memory area for p2m needed due to E820 map conflict\n"); BUG(); diff --git a/arch/x86/xen/setup.c b/arch/x86/xen/setup.c index f8960fc..513c48b 100644 --- a/arch/x86/xen/setup.c +++ b/arch/x86/xen/setup.c @@ -634,14 +634,15 @@ bool __init xen_is_e820_reserved(phys_addr_t start, phys_addr_t size) } /* - * Find a free area in physical memory not yet reserved and compliant with - * E820 map. + * Find a free area in physical memory not yet reserved, compliant with the + * E820 map and not overlapping with the pre-allocated area. * Used to relocate pre-allocated areas like initrd or p2m list which are in * conflict with the to be used E820 map. * In case no area is found, return 0. Otherwise return the physical address * of the area which is already reserved for convenience. */ -phys_addr_t __init xen_find_free_area(phys_addr_t size) +phys_addr_t __init xen_find_free_area(phys_addr_t size, phys_addr_t cur_start, + phys_addr_t cur_size) { unsigned mapcnt; phys_addr_t addr, start; @@ -652,7 +653,8 @@ phys_addr_t __init xen_find_free_area(phys_addr_t size) continue; start = entry->addr; for (addr = start; addr < start + size; addr += PAGE_SIZE) { - if (!memblock_is_reserved(addr)) + if (!memblock_is_reserved(addr) && + (addr < cur_start || addr >= cur_start + cur_size)) continue; start = addr + PAGE_SIZE; if (start + size > entry->addr + entry->size) @@ -726,7 +728,7 @@ static void __init xen_reserve_xen_mfnlist(void) xen_raw_console_write("Xen hypervisor allocated p2m list conflicts with E820 map\n"); BUG(); #else - xen_relocate_p2m(); + xen_relocate_p2m(start, size); #endif } @@ -887,7 +889,9 @@ char * __init xen_memory_setup(void) boot_params.hdr.ramdisk_size)) { phys_addr_t new_area, start, size; - new_area = xen_find_free_area(boot_params.hdr.ramdisk_size); + new_area = xen_find_free_area(boot_params.hdr.ramdisk_size, + boot_params.hdr.ramdisk_image, + boot_params.hdr.ramdisk_size); if (!new_area) { xen_raw_console_write("Can't find new memory area for initrd needed due to E820 map conflict\n"); BUG(); diff --git a/arch/x86/xen/xen-ops.h b/arch/x86/xen/xen-ops.h index 3cbce3b..d3342b8 100644 --- a/arch/x86/xen/xen-ops.h +++ b/arch/x86/xen/xen-ops.h @@ -41,14 +41,15 @@ void __init xen_pt_check_e820(void); void xen_mm_pin_all(void); void xen_mm_unpin_all(void); #ifdef CONFIG_X86_64 -void __init xen_relocate_p2m(void); +void __init xen_relocate_p2m(phys_addr_t cur_start, phys_addr_t cur_size); #endif bool __init xen_is_e820_reserved(phys_addr_t start, phys_addr_t size); unsigned long __ref xen_chk_extra_mem(unsigned long pfn); void __init xen_inv_extra_mem(void); void __init xen_remap_memory(void); -phys_addr_t __init xen_find_free_area(phys_addr_t size); +phys_addr_t __init xen_find_free_area(phys_addr_t size, phys_addr_t cur_start, + phys_addr_t cur_size); char * __init xen_memory_setup(void); char * xen_auto_xlated_memory_setup(void); void __init xen_arch_setup(void);
When relocating the p2m/initrd, take special care not to relocate it so that is overlaps with the current location of the p2m/initrd. This is needed since the full extent of the current location is not marked as a reserved region in the e820 (and it shouldn't be since it is about to be moved). This was seen to happen to a dom0 with a large initial p2m and a small reserved region in the middle of the initial p2m. Signed-off-by: Ross Lagerwall <ross.lagerwall@citrix.com> --- arch/x86/xen/mmu.c | 4 ++-- arch/x86/xen/setup.c | 16 ++++++++++------ arch/x86/xen/xen-ops.h | 5 +++-- 3 files changed, 15 insertions(+), 10 deletions(-)