* [PATCH] xen/setup: Don't relocate p2m/initrd over existing one
@ 2016-12-09 15:50 Ross Lagerwall
2016-12-09 21:08 ` Boris Ostrovsky
2016-12-12 8:19 ` Juergen Gross
0 siblings, 2 replies; 6+ messages in thread
From: Ross Lagerwall @ 2016-12-09 15:50 UTC (permalink / raw)
To: xen-devel; +Cc: Juergen Gross, Ross Lagerwall, Boris Ostrovsky, David Vrabel
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(-)
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);
--
2.7.4
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] xen/setup: Don't relocate p2m/initrd over existing one
2016-12-09 15:50 [PATCH] xen/setup: Don't relocate p2m/initrd over existing one Ross Lagerwall
@ 2016-12-09 21:08 ` Boris Ostrovsky
2016-12-12 7:28 ` Ross Lagerwall
2016-12-12 8:19 ` Juergen Gross
1 sibling, 1 reply; 6+ messages in thread
From: Boris Ostrovsky @ 2016-12-09 21:08 UTC (permalink / raw)
To: Ross Lagerwall, xen-devel; +Cc: Juergen Gross, David Vrabel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xen/setup: Don't relocate p2m/initrd over existing one
2016-12-09 21:08 ` Boris Ostrovsky
@ 2016-12-12 7:28 ` Ross Lagerwall
0 siblings, 0 replies; 6+ messages in thread
From: Ross Lagerwall @ 2016-12-12 7:28 UTC (permalink / raw)
To: Boris Ostrovsky; +Cc: Juergen Gross, xen-devel, David Vrabel
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.
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/setup: Don't relocate p2m/initrd over existing one
2016-12-09 15:50 [PATCH] xen/setup: Don't relocate p2m/initrd over existing one Ross Lagerwall
2016-12-09 21:08 ` Boris Ostrovsky
@ 2016-12-12 8:19 ` Juergen Gross
2016-12-12 8:28 ` Ross Lagerwall
1 sibling, 1 reply; 6+ messages in thread
From: Juergen Gross @ 2016-12-12 8:19 UTC (permalink / raw)
To: Ross Lagerwall, xen-devel; +Cc: Boris Ostrovsky, David Vrabel
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);
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] xen/setup: Don't relocate p2m/initrd over existing one
2016-12-12 8:19 ` Juergen Gross
@ 2016-12-12 8:28 ` Ross Lagerwall
2016-12-12 8:37 ` Juergen Gross
0 siblings, 1 reply; 6+ messages in thread
From: Ross Lagerwall @ 2016-12-12 8:28 UTC (permalink / raw)
To: Juergen Gross; +Cc: xen-devel, Boris Ostrovsky, David Vrabel
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.
--
Ross Lagerwall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen/setup: Don't relocate p2m/initrd over existing one
2016-12-12 8:28 ` Ross Lagerwall
@ 2016-12-12 8:37 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2016-12-12 8:37 UTC (permalink / raw)
To: Ross Lagerwall; +Cc: xen-devel, Boris Ostrovsky, David Vrabel
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
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-12-12 8:37 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-12-09 15:50 [PATCH] xen/setup: Don't relocate p2m/initrd over existing one Ross Lagerwall
2016-12-09 21:08 ` Boris Ostrovsky
2016-12-12 7:28 ` Ross Lagerwall
2016-12-12 8:19 ` Juergen Gross
2016-12-12 8:28 ` Ross Lagerwall
2016-12-12 8:37 ` Juergen Gross
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).