* [PATCH] x86: EFI stub support for large memory maps
@ 2013-08-23 21:02 Linn Crosetto
2013-09-06 10:03 ` Matt Fleming
2013-09-06 17:04 ` [PATCHv2] " Linn Crosetto
0 siblings, 2 replies; 11+ messages in thread
From: Linn Crosetto @ 2013-08-23 21:02 UTC (permalink / raw)
To: matt.fleming, hpa, tglx, mingo, x86, yinghai
Cc: linux-efi, linux-kernel, Linn Crosetto
This patch fixes a problem with EFI memory maps larger than 128 entries
when booting using the EFI stub, which results in overflowing e820_map
in boot_params and an eventual halt when checking the map size in
sanitize_e820_map().
If the number of map entries is greater than what can fit in e820_map,
add the extra entries to the setup_data list using type SETUP_E820_EXT.
These extra entries are then picked up when the setup_data list is
parsed in parse_e820_ext().
Signed-off-by: Linn Crosetto <linn@hp.com>
---
arch/x86/boot/compressed/eboot.c | 220 ++++++++++++++++++++++++++++-----------
1 file changed, 157 insertions(+), 63 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..89fcfe6 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -982,20 +982,158 @@ fail:
return NULL;
}
+static void add_e820ext(struct boot_params *params,
+ struct setup_data *e820ext, u32 nr_entries)
+{
+ struct setup_data *data;
+ efi_status_t status;
+ unsigned long size;
+
+ e820ext->type = SETUP_E820_EXT;
+ e820ext->len = nr_entries * sizeof(struct e820entry);
+ e820ext->next = 0;
+
+ data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
+
+ while (data && data->next)
+ data = (struct setup_data *)(unsigned long)data->next;
+
+ if (data)
+ data->next = (unsigned long)e820ext;
+ else
+ params->hdr.setup_data = (unsigned long)e820ext;
+}
+
+static efi_status_t setup_e820(struct boot_params *params,
+ struct setup_data *e820ext, u32 e820ext_size)
+{
+ struct e820entry *e820_map = ¶ms->e820_map[0];
+ struct efi_info *efi = ¶ms->efi_info;
+ struct e820entry *prev = NULL;
+ u32 nr_entries;
+ u32 nr_desc;
+ int i;
+
+ nr_entries = 0;
+ nr_desc = efi->efi_memmap_size / efi->efi_memdesc_size;
+
+ for (i = 0; i < nr_desc; i++) {
+ efi_memory_desc_t *d;
+ unsigned int e820_type = 0;
+ unsigned long m = efi->efi_memmap;
+
+ d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
+ switch (d->type) {
+ case EFI_RESERVED_TYPE:
+ case EFI_RUNTIME_SERVICES_CODE:
+ case EFI_RUNTIME_SERVICES_DATA:
+ case EFI_MEMORY_MAPPED_IO:
+ case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
+ case EFI_PAL_CODE:
+ e820_type = E820_RESERVED;
+ break;
+
+ case EFI_UNUSABLE_MEMORY:
+ e820_type = E820_UNUSABLE;
+ break;
+
+ case EFI_ACPI_RECLAIM_MEMORY:
+ e820_type = E820_ACPI;
+ break;
+
+ case EFI_LOADER_CODE:
+ case EFI_LOADER_DATA:
+ case EFI_BOOT_SERVICES_CODE:
+ case EFI_BOOT_SERVICES_DATA:
+ case EFI_CONVENTIONAL_MEMORY:
+ e820_type = E820_RAM;
+ break;
+
+ case EFI_ACPI_MEMORY_NVS:
+ e820_type = E820_NVS;
+ break;
+
+ default:
+ continue;
+ }
+
+ /* Merge adjacent mappings */
+ if (prev && prev->type == e820_type &&
+ (prev->addr + prev->size) == d->phys_addr) {
+ prev->size += d->num_pages << 12;
+ continue;
+ }
+
+ if (nr_entries == ARRAY_SIZE(params->e820_map)) {
+ u32 need = (nr_desc - i) * sizeof(struct e820entry) +
+ sizeof(struct setup_data);
+
+ if (!e820ext || e820ext_size < need)
+ return EFI_BUFFER_TOO_SMALL;
+
+ /* boot_params map full, switch to e820 extended */
+ e820_map = (struct e820entry *)e820ext->data;
+ }
+
+ e820_map->addr = d->phys_addr;
+ e820_map->size = d->num_pages << PAGE_SHIFT;
+ e820_map->type = e820_type;
+ prev = e820_map++;
+ nr_entries++;
+ }
+
+ if (nr_entries > ARRAY_SIZE(params->e820_map)) {
+ u32 nr_e820ext = nr_entries - ARRAY_SIZE(params->e820_map);
+
+ add_e820ext(params, e820ext, nr_e820ext);
+ nr_entries -= nr_e820ext;
+ }
+
+ params->e820_entries = (u8)nr_entries;
+
+ return EFI_SUCCESS;
+}
+
+static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
+ u32 *e820ext_size)
+{
+ efi_status_t status;
+ unsigned long size;
+
+ size = sizeof(struct setup_data) +
+ sizeof(struct e820entry) * nr_desc;
+
+ if (*e820ext && size <= *e820ext_size)
+ return EFI_SUCCESS; /* Already allocated */
+
+ if (*e820ext)
+ efi_call_phys1(sys_table->boottime->free_pool, *e820ext);
+
+ status = efi_call_phys3(sys_table->boottime->allocate_pool,
+ EFI_LOADER_DATA, size, e820ext);
+
+ if (status == EFI_SUCCESS)
+ *e820ext_size = size;
+
+ return status;
+}
+
static efi_status_t exit_boot(struct boot_params *boot_params,
void *handle)
{
struct efi_info *efi = &boot_params->efi_info;
- struct e820entry *e820_map = &boot_params->e820_map[0];
- struct e820entry *prev = NULL;
unsigned long size, key, desc_size, _size;
efi_memory_desc_t *mem_map;
+ struct setup_data *e820ext;
+ __u32 e820ext_size;
+ __u32 nr_desc, prev_nr_desc;
efi_status_t status;
__u32 desc_version;
bool called_exit = false;
- u8 nr_entries;
- int i;
+ nr_desc = 0;
+ e820ext = NULL;
+ e820ext_size = 0;
size = sizeof(*mem_map) * 32;
again:
@@ -1016,6 +1154,19 @@ get_map:
if (status != EFI_SUCCESS)
goto free_mem_map;
+ prev_nr_desc = nr_desc;
+ nr_desc = size / desc_size;
+ if (nr_desc > prev_nr_desc &&
+ nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
+ u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
+
+ status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
+ if (status != EFI_SUCCESS)
+ return status;
+
+ goto get_map; /* Allocated memory, get map again */
+ }
+
memcpy(&efi->efi_loader_signature, EFI_LOADER_SIGNATURE, sizeof(__u32));
efi->efi_systab = (unsigned long)sys_table;
efi->efi_memdesc_size = desc_size;
@@ -1049,66 +1200,9 @@ get_map:
/* Historic? */
boot_params->alt_mem_k = 32 * 1024;
- /*
- * Convert the EFI memory map to E820.
- */
- nr_entries = 0;
- for (i = 0; i < size / desc_size; i++) {
- efi_memory_desc_t *d;
- unsigned int e820_type = 0;
- unsigned long m = (unsigned long)mem_map;
-
- d = (efi_memory_desc_t *)(m + (i * desc_size));
- switch (d->type) {
- case EFI_RESERVED_TYPE:
- case EFI_RUNTIME_SERVICES_CODE:
- case EFI_RUNTIME_SERVICES_DATA:
- case EFI_MEMORY_MAPPED_IO:
- case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
- case EFI_PAL_CODE:
- e820_type = E820_RESERVED;
- break;
-
- case EFI_UNUSABLE_MEMORY:
- e820_type = E820_UNUSABLE;
- break;
-
- case EFI_ACPI_RECLAIM_MEMORY:
- e820_type = E820_ACPI;
- break;
-
- case EFI_LOADER_CODE:
- case EFI_LOADER_DATA:
- case EFI_BOOT_SERVICES_CODE:
- case EFI_BOOT_SERVICES_DATA:
- case EFI_CONVENTIONAL_MEMORY:
- e820_type = E820_RAM;
- break;
-
- case EFI_ACPI_MEMORY_NVS:
- e820_type = E820_NVS;
- break;
-
- default:
- continue;
- }
-
- /* Merge adjacent mappings */
- if (prev && prev->type == e820_type &&
- (prev->addr + prev->size) == d->phys_addr)
- prev->size += d->num_pages << 12;
- else {
- e820_map->addr = d->phys_addr;
- e820_map->size = d->num_pages << 12;
- e820_map->type = e820_type;
- prev = e820_map++;
- nr_entries++;
- }
- }
-
- boot_params->e820_entries = nr_entries;
+ status = setup_e820(boot_params, e820ext, e820ext_size);
- return EFI_SUCCESS;
+ return status;
free_mem_map:
low_free(_size, (unsigned long)mem_map);
--
1.7.11.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] x86: EFI stub support for large memory maps
2013-08-23 21:02 [PATCH] x86: EFI stub support for large memory maps Linn Crosetto
@ 2013-09-06 10:03 ` Matt Fleming
2013-09-06 17:04 ` [PATCHv2] " Linn Crosetto
1 sibling, 0 replies; 11+ messages in thread
From: Matt Fleming @ 2013-09-06 10:03 UTC (permalink / raw)
To: Linn Crosetto
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Fri, 23 Aug, at 03:02:51PM, Linn Crosetto wrote:
> This patch fixes a problem with EFI memory maps larger than 128 entries
> when booting using the EFI stub, which results in overflowing e820_map
> in boot_params and an eventual halt when checking the map size in
> sanitize_e820_map().
>
> If the number of map entries is greater than what can fit in e820_map,
> add the extra entries to the setup_data list using type SETUP_E820_EXT.
> These extra entries are then picked up when the setup_data list is
> parsed in parse_e820_ext().
>
> Signed-off-by: Linn Crosetto <linn@hp.com>
> ---
> arch/x86/boot/compressed/eboot.c | 220 ++++++++++++++++++++++++++++-----------
> 1 file changed, 157 insertions(+), 63 deletions(-)
[...]
> @@ -1016,6 +1154,19 @@ get_map:
> if (status != EFI_SUCCESS)
> goto free_mem_map;
>
> + prev_nr_desc = nr_desc;
> + nr_desc = size / desc_size;
> + if (nr_desc > prev_nr_desc &&
> + nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
> + u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
> +
> + status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
> + if (status != EFI_SUCCESS)
> + return status;
In the error path shouldn't we jump to free_mem_map to avoid leaking
memory?
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv2] x86: EFI stub support for large memory maps
2013-08-23 21:02 [PATCH] x86: EFI stub support for large memory maps Linn Crosetto
2013-09-06 10:03 ` Matt Fleming
@ 2013-09-06 17:04 ` Linn Crosetto
2013-09-17 20:14 ` Matt Fleming
2013-09-23 1:59 ` [PATCHv3] " Linn Crosetto
1 sibling, 2 replies; 11+ messages in thread
From: Linn Crosetto @ 2013-09-06 17:04 UTC (permalink / raw)
To: matt.fleming, hpa, tglx, mingo, x86, yinghai
Cc: linux-efi, linux-kernel, Linn Crosetto
This patch fixes a problem with EFI memory maps larger than 128 entries
when booting using the EFI stub, which results in overflowing e820_map
in boot_params and an eventual halt when checking the map size in
sanitize_e820_map().
If the number of map entries is greater than what can fit in e820_map,
add the extra entries to the setup_data list using type SETUP_E820_EXT.
These extra entries are then picked up when the setup_data list is
parsed in parse_e820_ext().
Signed-off-by: Linn Crosetto <linn@hp.com>
---
Changes in v2:
* Free memory when error is returned from alloc_e820ext() as suggested by Matt
Fleming
* Set pointer to NULL and size to 0 after freeing memory in alloc_e820ext()
arch/x86/boot/compressed/eboot.c | 223 ++++++++++++++++++++++++++++-----------
1 file changed, 160 insertions(+), 63 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..2ac395e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -982,20 +982,161 @@ fail:
return NULL;
}
+static void add_e820ext(struct boot_params *params,
+ struct setup_data *e820ext, u32 nr_entries)
+{
+ struct setup_data *data;
+ efi_status_t status;
+ unsigned long size;
+
+ e820ext->type = SETUP_E820_EXT;
+ e820ext->len = nr_entries * sizeof(struct e820entry);
+ e820ext->next = 0;
+
+ data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
+
+ while (data && data->next)
+ data = (struct setup_data *)(unsigned long)data->next;
+
+ if (data)
+ data->next = (unsigned long)e820ext;
+ else
+ params->hdr.setup_data = (unsigned long)e820ext;
+}
+
+static efi_status_t setup_e820(struct boot_params *params,
+ struct setup_data *e820ext, u32 e820ext_size)
+{
+ struct e820entry *e820_map = ¶ms->e820_map[0];
+ struct efi_info *efi = ¶ms->efi_info;
+ struct e820entry *prev = NULL;
+ u32 nr_entries;
+ u32 nr_desc;
+ int i;
+
+ nr_entries = 0;
+ nr_desc = efi->efi_memmap_size / efi->efi_memdesc_size;
+
+ for (i = 0; i < nr_desc; i++) {
+ efi_memory_desc_t *d;
+ unsigned int e820_type = 0;
+ unsigned long m = efi->efi_memmap;
+
+ d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
+ switch (d->type) {
+ case EFI_RESERVED_TYPE:
+ case EFI_RUNTIME_SERVICES_CODE:
+ case EFI_RUNTIME_SERVICES_DATA:
+ case EFI_MEMORY_MAPPED_IO:
+ case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
+ case EFI_PAL_CODE:
+ e820_type = E820_RESERVED;
+ break;
+
+ case EFI_UNUSABLE_MEMORY:
+ e820_type = E820_UNUSABLE;
+ break;
+
+ case EFI_ACPI_RECLAIM_MEMORY:
+ e820_type = E820_ACPI;
+ break;
+
+ case EFI_LOADER_CODE:
+ case EFI_LOADER_DATA:
+ case EFI_BOOT_SERVICES_CODE:
+ case EFI_BOOT_SERVICES_DATA:
+ case EFI_CONVENTIONAL_MEMORY:
+ e820_type = E820_RAM;
+ break;
+
+ case EFI_ACPI_MEMORY_NVS:
+ e820_type = E820_NVS;
+ break;
+
+ default:
+ continue;
+ }
+
+ /* Merge adjacent mappings */
+ if (prev && prev->type == e820_type &&
+ (prev->addr + prev->size) == d->phys_addr) {
+ prev->size += d->num_pages << 12;
+ continue;
+ }
+
+ if (nr_entries == ARRAY_SIZE(params->e820_map)) {
+ u32 need = (nr_desc - i) * sizeof(struct e820entry) +
+ sizeof(struct setup_data);
+
+ if (!e820ext || e820ext_size < need)
+ return EFI_BUFFER_TOO_SMALL;
+
+ /* boot_params map full, switch to e820 extended */
+ e820_map = (struct e820entry *)e820ext->data;
+ }
+
+ e820_map->addr = d->phys_addr;
+ e820_map->size = d->num_pages << PAGE_SHIFT;
+ e820_map->type = e820_type;
+ prev = e820_map++;
+ nr_entries++;
+ }
+
+ if (nr_entries > ARRAY_SIZE(params->e820_map)) {
+ u32 nr_e820ext = nr_entries - ARRAY_SIZE(params->e820_map);
+
+ add_e820ext(params, e820ext, nr_e820ext);
+ nr_entries -= nr_e820ext;
+ }
+
+ params->e820_entries = (u8)nr_entries;
+
+ return EFI_SUCCESS;
+}
+
+static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
+ u32 *e820ext_size)
+{
+ efi_status_t status;
+ unsigned long size;
+
+ size = sizeof(struct setup_data) +
+ sizeof(struct e820entry) * nr_desc;
+
+ if (*e820ext && size <= *e820ext_size)
+ return EFI_SUCCESS; /* Already allocated */
+
+ if (*e820ext) {
+ efi_call_phys1(sys_table->boottime->free_pool, *e820ext);
+ *e820ext = NULL;
+ *e820ext_size = 0;
+ }
+
+ status = efi_call_phys3(sys_table->boottime->allocate_pool,
+ EFI_LOADER_DATA, size, e820ext);
+
+ if (status == EFI_SUCCESS)
+ *e820ext_size = size;
+
+ return status;
+}
+
static efi_status_t exit_boot(struct boot_params *boot_params,
void *handle)
{
struct efi_info *efi = &boot_params->efi_info;
- struct e820entry *e820_map = &boot_params->e820_map[0];
- struct e820entry *prev = NULL;
unsigned long size, key, desc_size, _size;
efi_memory_desc_t *mem_map;
+ struct setup_data *e820ext;
+ __u32 e820ext_size;
+ __u32 nr_desc, prev_nr_desc;
efi_status_t status;
__u32 desc_version;
bool called_exit = false;
- u8 nr_entries;
- int i;
+ nr_desc = 0;
+ e820ext = NULL;
+ e820ext_size = 0;
size = sizeof(*mem_map) * 32;
again:
@@ -1016,6 +1157,19 @@ get_map:
if (status != EFI_SUCCESS)
goto free_mem_map;
+ prev_nr_desc = nr_desc;
+ nr_desc = size / desc_size;
+ if (nr_desc > prev_nr_desc &&
+ nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
+ u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
+
+ status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
+ if (status != EFI_SUCCESS)
+ goto free_mem_map;
+
+ goto get_map; /* Allocated memory, get map again */
+ }
+
memcpy(&efi->efi_loader_signature, EFI_LOADER_SIGNATURE, sizeof(__u32));
efi->efi_systab = (unsigned long)sys_table;
efi->efi_memdesc_size = desc_size;
@@ -1049,66 +1203,9 @@ get_map:
/* Historic? */
boot_params->alt_mem_k = 32 * 1024;
- /*
- * Convert the EFI memory map to E820.
- */
- nr_entries = 0;
- for (i = 0; i < size / desc_size; i++) {
- efi_memory_desc_t *d;
- unsigned int e820_type = 0;
- unsigned long m = (unsigned long)mem_map;
+ status = setup_e820(boot_params, e820ext, e820ext_size);
- d = (efi_memory_desc_t *)(m + (i * desc_size));
- switch (d->type) {
- case EFI_RESERVED_TYPE:
- case EFI_RUNTIME_SERVICES_CODE:
- case EFI_RUNTIME_SERVICES_DATA:
- case EFI_MEMORY_MAPPED_IO:
- case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
- case EFI_PAL_CODE:
- e820_type = E820_RESERVED;
- break;
-
- case EFI_UNUSABLE_MEMORY:
- e820_type = E820_UNUSABLE;
- break;
-
- case EFI_ACPI_RECLAIM_MEMORY:
- e820_type = E820_ACPI;
- break;
-
- case EFI_LOADER_CODE:
- case EFI_LOADER_DATA:
- case EFI_BOOT_SERVICES_CODE:
- case EFI_BOOT_SERVICES_DATA:
- case EFI_CONVENTIONAL_MEMORY:
- e820_type = E820_RAM;
- break;
-
- case EFI_ACPI_MEMORY_NVS:
- e820_type = E820_NVS;
- break;
-
- default:
- continue;
- }
-
- /* Merge adjacent mappings */
- if (prev && prev->type == e820_type &&
- (prev->addr + prev->size) == d->phys_addr)
- prev->size += d->num_pages << 12;
- else {
- e820_map->addr = d->phys_addr;
- e820_map->size = d->num_pages << 12;
- e820_map->type = e820_type;
- prev = e820_map++;
- nr_entries++;
- }
- }
-
- boot_params->e820_entries = nr_entries;
-
- return EFI_SUCCESS;
+ return status;
free_mem_map:
low_free(_size, (unsigned long)mem_map);
--
1.7.11.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv2] x86: EFI stub support for large memory maps
2013-09-06 17:04 ` [PATCHv2] " Linn Crosetto
@ 2013-09-17 20:14 ` Matt Fleming
2013-09-23 0:23 ` Linn Crosetto
2013-09-23 1:59 ` [PATCHv3] " Linn Crosetto
1 sibling, 1 reply; 11+ messages in thread
From: Matt Fleming @ 2013-09-17 20:14 UTC (permalink / raw)
To: Linn Crosetto
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Fri, 06 Sep, at 11:04:16AM, Linn Crosetto wrote:
> This patch fixes a problem with EFI memory maps larger than 128 entries
> when booting using the EFI stub, which results in overflowing e820_map
> in boot_params and an eventual halt when checking the map size in
> sanitize_e820_map().
>
> If the number of map entries is greater than what can fit in e820_map,
> add the extra entries to the setup_data list using type SETUP_E820_EXT.
> These extra entries are then picked up when the setup_data list is
> parsed in parse_e820_ext().
>
> Signed-off-by: Linn Crosetto <linn@hp.com>
> ---
> Changes in v2:
> * Free memory when error is returned from alloc_e820ext() as suggested by Matt
> Fleming
> * Set pointer to NULL and size to 0 after freeing memory in alloc_e820ext()
>
> arch/x86/boot/compressed/eboot.c | 223 ++++++++++++++++++++++++++++-----------
> 1 file changed, 160 insertions(+), 63 deletions(-)
[...]
> +static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
> + u32 *e820ext_size)
> +{
> + efi_status_t status;
> + unsigned long size;
> +
> + size = sizeof(struct setup_data) +
> + sizeof(struct e820entry) * nr_desc;
> +
> + if (*e820ext && size <= *e820ext_size)
> + return EFI_SUCCESS; /* Already allocated */
Do we actually need this check? I thought the 'prev_nr_desc' below
ensures we only allocate 'e820ext' if we need more memory.
[...]
> @@ -1016,6 +1157,19 @@ get_map:
> if (status != EFI_SUCCESS)
> goto free_mem_map;
>
> + prev_nr_desc = nr_desc;
> + nr_desc = size / desc_size;
> + if (nr_desc > prev_nr_desc &&
> + nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
> + u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
> +
> + status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
> + if (status != EFI_SUCCESS)
> + goto free_mem_map;
> +
> + goto get_map; /* Allocated memory, get map again */
> + }
> +
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv2] x86: EFI stub support for large memory maps
2013-09-17 20:14 ` Matt Fleming
@ 2013-09-23 0:23 ` Linn Crosetto
0 siblings, 0 replies; 11+ messages in thread
From: Linn Crosetto @ 2013-09-23 0:23 UTC (permalink / raw)
To: Matt Fleming
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Tue, Sep 17, 2013 at 09:14:52PM +0100, Matt Fleming wrote:
> > +static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
> > + u32 *e820ext_size)
> > +{
> > + efi_status_t status;
> > + unsigned long size;
> > +
> > + size = sizeof(struct setup_data) +
> > + sizeof(struct e820entry) * nr_desc;
> > +
> > + if (*e820ext && size <= *e820ext_size)
> > + return EFI_SUCCESS; /* Already allocated */
>
> Do we actually need this check? I thought the 'prev_nr_desc' below
> ensures we only allocate 'e820ext' if we need more memory.
I am okay with removing the check. There is another bug in exit_boot, when
jumping to get_map the call to get the memory map is passed the previous map
size instead of the size of the allocated buffer. I'll make the changes and
resend.
>
> [...]
>
> > @@ -1016,6 +1157,19 @@ get_map:
> > if (status != EFI_SUCCESS)
> > goto free_mem_map;
> >
> > + prev_nr_desc = nr_desc;
> > + nr_desc = size / desc_size;
> > + if (nr_desc > prev_nr_desc &&
> > + nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
> > + u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
> > +
> > + status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
> > + if (status != EFI_SUCCESS)
> > + goto free_mem_map;
> > +
> > + goto get_map; /* Allocated memory, get map again */
> > + }
> > +
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCHv3] x86: EFI stub support for large memory maps
2013-09-06 17:04 ` [PATCHv2] " Linn Crosetto
2013-09-17 20:14 ` Matt Fleming
@ 2013-09-23 1:59 ` Linn Crosetto
2013-09-25 12:58 ` Matt Fleming
1 sibling, 1 reply; 11+ messages in thread
From: Linn Crosetto @ 2013-09-23 1:59 UTC (permalink / raw)
To: matt.fleming, hpa, tglx, mingo, x86, yinghai
Cc: linux-efi, linux-kernel, Linn Crosetto
This patch fixes a problem with EFI memory maps larger than 128 entries
when booting using the EFI stub, which results in overflowing e820_map
in boot_params and an eventual halt when checking the map size in
sanitize_e820_map().
If the number of map entries is greater than what can fit in e820_map,
add the extra entries to the setup_data list using type SETUP_E820_EXT.
These extra entries are then picked up when the setup_data list is
parsed in parse_e820_ext().
Signed-off-by: Linn Crosetto <linn@hp.com>
---
Changes from v2:
* Removed unnecessary optimization in alloc_e820ext() (Matt Fleming)
* Fixed a bug where an incorrect buffer size may be passed to
get_memory_map when jumping to get_map
arch/x86/boot/compressed/eboot.c | 239 +++++++++++++++++++++++++++------------
1 file changed, 167 insertions(+), 72 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index b7388a4..bfa177a 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -982,46 +982,198 @@ fail:
return NULL;
}
+static void add_e820ext(struct boot_params *params,
+ struct setup_data *e820ext, u32 nr_entries)
+{
+ struct setup_data *data;
+ efi_status_t status;
+ unsigned long size;
+
+ e820ext->type = SETUP_E820_EXT;
+ e820ext->len = nr_entries * sizeof(struct e820entry);
+ e820ext->next = 0;
+
+ data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
+
+ while (data && data->next)
+ data = (struct setup_data *)(unsigned long)data->next;
+
+ if (data)
+ data->next = (unsigned long)e820ext;
+ else
+ params->hdr.setup_data = (unsigned long)e820ext;
+}
+
+static efi_status_t setup_e820(struct boot_params *params,
+ struct setup_data *e820ext, u32 e820ext_size)
+{
+ struct e820entry *e820_map = ¶ms->e820_map[0];
+ struct efi_info *efi = ¶ms->efi_info;
+ struct e820entry *prev = NULL;
+ u32 nr_entries;
+ u32 nr_desc;
+ int i;
+
+ nr_entries = 0;
+ nr_desc = efi->efi_memmap_size / efi->efi_memdesc_size;
+
+ for (i = 0; i < nr_desc; i++) {
+ efi_memory_desc_t *d;
+ unsigned int e820_type = 0;
+ unsigned long m = efi->efi_memmap;
+
+ d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
+ switch (d->type) {
+ case EFI_RESERVED_TYPE:
+ case EFI_RUNTIME_SERVICES_CODE:
+ case EFI_RUNTIME_SERVICES_DATA:
+ case EFI_MEMORY_MAPPED_IO:
+ case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
+ case EFI_PAL_CODE:
+ e820_type = E820_RESERVED;
+ break;
+
+ case EFI_UNUSABLE_MEMORY:
+ e820_type = E820_UNUSABLE;
+ break;
+
+ case EFI_ACPI_RECLAIM_MEMORY:
+ e820_type = E820_ACPI;
+ break;
+
+ case EFI_LOADER_CODE:
+ case EFI_LOADER_DATA:
+ case EFI_BOOT_SERVICES_CODE:
+ case EFI_BOOT_SERVICES_DATA:
+ case EFI_CONVENTIONAL_MEMORY:
+ e820_type = E820_RAM;
+ break;
+
+ case EFI_ACPI_MEMORY_NVS:
+ e820_type = E820_NVS;
+ break;
+
+ default:
+ continue;
+ }
+
+ /* Merge adjacent mappings */
+ if (prev && prev->type == e820_type &&
+ (prev->addr + prev->size) == d->phys_addr) {
+ prev->size += d->num_pages << 12;
+ continue;
+ }
+
+ if (nr_entries == ARRAY_SIZE(params->e820_map)) {
+ u32 need = (nr_desc - i) * sizeof(struct e820entry) +
+ sizeof(struct setup_data);
+
+ if (!e820ext || e820ext_size < need)
+ return EFI_BUFFER_TOO_SMALL;
+
+ /* boot_params map full, switch to e820 extended */
+ e820_map = (struct e820entry *)e820ext->data;
+ }
+
+ e820_map->addr = d->phys_addr;
+ e820_map->size = d->num_pages << PAGE_SHIFT;
+ e820_map->type = e820_type;
+ prev = e820_map++;
+ nr_entries++;
+ }
+
+ if (nr_entries > ARRAY_SIZE(params->e820_map)) {
+ u32 nr_e820ext = nr_entries - ARRAY_SIZE(params->e820_map);
+
+ add_e820ext(params, e820ext, nr_e820ext);
+ nr_entries -= nr_e820ext;
+ }
+
+ params->e820_entries = (u8)nr_entries;
+
+ return EFI_SUCCESS;
+}
+
+static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
+ u32 *e820ext_size)
+{
+ efi_status_t status;
+ unsigned long size;
+
+ size = sizeof(struct setup_data) +
+ sizeof(struct e820entry) * nr_desc;
+
+ if (*e820ext) {
+ efi_call_phys1(sys_table->boottime->free_pool, *e820ext);
+ *e820ext = NULL;
+ *e820ext_size = 0;
+ }
+
+ status = efi_call_phys3(sys_table->boottime->allocate_pool,
+ EFI_LOADER_DATA, size, e820ext);
+
+ if (status == EFI_SUCCESS)
+ *e820ext_size = size;
+
+ return status;
+}
+
static efi_status_t exit_boot(struct boot_params *boot_params,
void *handle)
{
struct efi_info *efi = &boot_params->efi_info;
- struct e820entry *e820_map = &boot_params->e820_map[0];
- struct e820entry *prev = NULL;
- unsigned long size, key, desc_size, _size;
+ unsigned long map_sz, key, desc_size, buf_sz;
efi_memory_desc_t *mem_map;
+ struct setup_data *e820ext;
+ __u32 e820ext_size;
+ __u32 nr_desc, prev_nr_desc;
efi_status_t status;
__u32 desc_version;
bool called_exit = false;
- u8 nr_entries;
- int i;
- size = sizeof(*mem_map) * 32;
+ nr_desc = 0;
+ e820ext = NULL;
+ e820ext_size = 0;
+ map_sz = sizeof(*mem_map) * 32;
again:
- size += sizeof(*mem_map) * 2;
- _size = size;
- status = low_alloc(size, 1, (unsigned long *)&mem_map);
+ map_sz += sizeof(*mem_map) * 2;
+ buf_sz = map_sz;
+ status = low_alloc(buf_sz, 1, (unsigned long *)&mem_map);
if (status != EFI_SUCCESS)
return status;
get_map:
- status = efi_call_phys5(sys_table->boottime->get_memory_map, &size,
+ map_sz = buf_sz;
+ status = efi_call_phys5(sys_table->boottime->get_memory_map, &map_sz,
mem_map, &key, &desc_size, &desc_version);
if (status == EFI_BUFFER_TOO_SMALL) {
- low_free(_size, (unsigned long)mem_map);
+ low_free(buf_sz, (unsigned long)mem_map);
goto again;
}
if (status != EFI_SUCCESS)
goto free_mem_map;
+ prev_nr_desc = nr_desc;
+ nr_desc = map_sz / desc_size;
+ if (nr_desc > prev_nr_desc &&
+ nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
+ u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
+
+ status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
+ if (status != EFI_SUCCESS)
+ goto free_mem_map;
+
+ goto get_map; /* Allocated memory, get map again */
+ }
+
memcpy(&efi->efi_loader_signature, EFI_LOADER_SIGNATURE, sizeof(__u32));
efi->efi_systab = (unsigned long)sys_table;
efi->efi_memdesc_size = desc_size;
efi->efi_memdesc_version = desc_version;
efi->efi_memmap = (unsigned long)mem_map;
- efi->efi_memmap_size = size;
+ efi->efi_memmap_size = map_sz;
#ifdef CONFIG_X86_64
efi->efi_systab_hi = (unsigned long)sys_table >> 32;
@@ -1049,69 +1201,12 @@ get_map:
/* Historic? */
boot_params->alt_mem_k = 32 * 1024;
- /*
- * Convert the EFI memory map to E820.
- */
- nr_entries = 0;
- for (i = 0; i < size / desc_size; i++) {
- efi_memory_desc_t *d;
- unsigned int e820_type = 0;
- unsigned long m = (unsigned long)mem_map;
-
- d = (efi_memory_desc_t *)(m + (i * desc_size));
- switch (d->type) {
- case EFI_RESERVED_TYPE:
- case EFI_RUNTIME_SERVICES_CODE:
- case EFI_RUNTIME_SERVICES_DATA:
- case EFI_MEMORY_MAPPED_IO:
- case EFI_MEMORY_MAPPED_IO_PORT_SPACE:
- case EFI_PAL_CODE:
- e820_type = E820_RESERVED;
- break;
-
- case EFI_UNUSABLE_MEMORY:
- e820_type = E820_UNUSABLE;
- break;
-
- case EFI_ACPI_RECLAIM_MEMORY:
- e820_type = E820_ACPI;
- break;
-
- case EFI_LOADER_CODE:
- case EFI_LOADER_DATA:
- case EFI_BOOT_SERVICES_CODE:
- case EFI_BOOT_SERVICES_DATA:
- case EFI_CONVENTIONAL_MEMORY:
- e820_type = E820_RAM;
- break;
-
- case EFI_ACPI_MEMORY_NVS:
- e820_type = E820_NVS;
- break;
-
- default:
- continue;
- }
-
- /* Merge adjacent mappings */
- if (prev && prev->type == e820_type &&
- (prev->addr + prev->size) == d->phys_addr)
- prev->size += d->num_pages << 12;
- else {
- e820_map->addr = d->phys_addr;
- e820_map->size = d->num_pages << 12;
- e820_map->type = e820_type;
- prev = e820_map++;
- nr_entries++;
- }
- }
-
- boot_params->e820_entries = nr_entries;
+ status = setup_e820(boot_params, e820ext, e820ext_size);
- return EFI_SUCCESS;
+ return status;
free_mem_map:
- low_free(_size, (unsigned long)mem_map);
+ low_free(buf_sz, (unsigned long)mem_map);
return status;
}
--
1.7.11.3
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv3] x86: EFI stub support for large memory maps
2013-09-23 1:59 ` [PATCHv3] " Linn Crosetto
@ 2013-09-25 12:58 ` Matt Fleming
2013-09-25 22:45 ` Linn Crosetto
0 siblings, 1 reply; 11+ messages in thread
From: Matt Fleming @ 2013-09-25 12:58 UTC (permalink / raw)
To: Linn Crosetto
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Sun, 22 Sep, at 07:59:08PM, Linn Crosetto wrote:
> This patch fixes a problem with EFI memory maps larger than 128 entries
> when booting using the EFI stub, which results in overflowing e820_map
> in boot_params and an eventual halt when checking the map size in
> sanitize_e820_map().
>
> If the number of map entries is greater than what can fit in e820_map,
> add the extra entries to the setup_data list using type SETUP_E820_EXT.
> These extra entries are then picked up when the setup_data list is
> parsed in parse_e820_ext().
>
> Signed-off-by: Linn Crosetto <linn@hp.com>
> ---
> Changes from v2:
> * Removed unnecessary optimization in alloc_e820ext() (Matt Fleming)
> * Fixed a bug where an incorrect buffer size may be passed to
> get_memory_map when jumping to get_map
>
> arch/x86/boot/compressed/eboot.c | 239 +++++++++++++++++++++++++++------------
> 1 file changed, 167 insertions(+), 72 deletions(-)
Thanks Linn. I applied this to the 'next' branch at,
git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
but it required a bit of massaging to apply on top of the changes
already there. Could you confirm that my changes are OK? I've included
the modified commit below.
---
>From 81c9e1df196ad531edabe726a2d7a610fb04fdd6 Mon Sep 17 00:00:00 2001
From: Linn Crosetto <linn@hp.com>
Date: Sun, 22 Sep 2013 19:59:08 -0600
Subject: [PATCH] x86: EFI stub support for large memory maps
This patch fixes a problem with EFI memory maps larger than 128 entries
when booting using the EFI stub, which results in overflowing e820_map
in boot_params and an eventual halt when checking the map size in
sanitize_e820_map().
If the number of map entries is greater than what can fit in e820_map,
add the extra entries to the setup_data list using type SETUP_E820_EXT.
These extra entries are then picked up when the setup_data list is
parsed in parse_e820_ext().
Signed-off-by: Linn Crosetto <linn@hp.com>
Signed-off-by: Matt Fleming <matt.fleming@intel.com>
---
arch/x86/boot/compressed/eboot.c | 220 ++++++++++++++++++++++++++++-----------
1 file changed, 159 insertions(+), 61 deletions(-)
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index beb07a4..482006e 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -519,71 +519,47 @@ fail:
return NULL;
}
-static efi_status_t exit_boot(struct boot_params *boot_params,
- void *handle)
+static void add_e820ext(struct boot_params *params,
+ struct setup_data *e820ext, u32 nr_entries)
{
- struct efi_info *efi = &boot_params->efi_info;
- struct e820entry *e820_map = &boot_params->e820_map[0];
- struct e820entry *prev = NULL;
- unsigned long size, key, desc_size, _size;
- efi_memory_desc_t *mem_map;
+ struct setup_data *data;
efi_status_t status;
- __u32 desc_version;
- bool called_exit = false;
- u8 nr_entries;
- int i;
+ unsigned long size;
-get_map:
- status = efi_get_memory_map(sys_table, &mem_map, &size, &desc_size,
- &desc_version, &key);
+ e820ext->type = SETUP_E820_EXT;
+ e820ext->len = nr_entries * sizeof(struct e820entry);
+ e820ext->next = 0;
- if (status != EFI_SUCCESS)
- return status;
-
- memcpy(&efi->efi_loader_signature, EFI_LOADER_SIGNATURE, sizeof(__u32));
- efi->efi_systab = (unsigned long)sys_table;
- efi->efi_memdesc_size = desc_size;
- efi->efi_memdesc_version = desc_version;
- efi->efi_memmap = (unsigned long)mem_map;
- efi->efi_memmap_size = size;
-
-#ifdef CONFIG_X86_64
- efi->efi_systab_hi = (unsigned long)sys_table >> 32;
- efi->efi_memmap_hi = (unsigned long)mem_map >> 32;
-#endif
+ data = (struct setup_data *)(unsigned long)params->hdr.setup_data;
- /* Might as well exit boot services now */
- status = efi_call_phys2(sys_table->boottime->exit_boot_services,
- handle, key);
- if (status != EFI_SUCCESS) {
- /*
- * ExitBootServices() will fail if any of the event
- * handlers change the memory map. In which case, we
- * must be prepared to retry, but only once so that
- * we're guaranteed to exit on repeated failures instead
- * of spinning forever.
- */
- if (called_exit)
- goto free_mem_map;
+ while (data && data->next)
+ data = (struct setup_data *)(unsigned long)data->next;
- called_exit = true;
- efi_call_phys1(sys_table->boottime->free_pool, mem_map);
- goto get_map;
- }
+ if (data)
+ data->next = (unsigned long)e820ext;
+ else
+ params->hdr.setup_data = (unsigned long)e820ext;
+}
- /* Historic? */
- boot_params->alt_mem_k = 32 * 1024;
+static efi_status_t setup_e820(struct boot_params *params,
+ struct setup_data *e820ext, u32 e820ext_size)
+{
+ struct e820entry *e820_map = ¶ms->e820_map[0];
+ struct efi_info *efi = ¶ms->efi_info;
+ struct e820entry *prev = NULL;
+ u32 nr_entries;
+ u32 nr_desc;
+ int i;
- /*
- * Convert the EFI memory map to E820.
- */
nr_entries = 0;
- for (i = 0; i < size / desc_size; i++) {
+ nr_desc = efi->efi_memmap_size / efi->efi_memdesc_size;
+
+ for (i = 0; i < nr_desc; i++) {
efi_memory_desc_t *d;
unsigned int e820_type = 0;
- unsigned long m = (unsigned long)mem_map;
+ unsigned long m = efi->efi_memmap;
- d = (efi_memory_desc_t *)(m + (i * desc_size));
+ d = (efi_memory_desc_t *)(m + (i * efi->efi_memdesc_size));
switch (d->type) {
case EFI_RESERVED_TYPE:
case EFI_RUNTIME_SERVICES_CODE:
@@ -620,18 +596,140 @@ get_map:
/* Merge adjacent mappings */
if (prev && prev->type == e820_type &&
- (prev->addr + prev->size) == d->phys_addr)
+ (prev->addr + prev->size) == d->phys_addr) {
prev->size += d->num_pages << 12;
- else {
- e820_map->addr = d->phys_addr;
- e820_map->size = d->num_pages << 12;
- e820_map->type = e820_type;
- prev = e820_map++;
- nr_entries++;
+ continue;
}
+
+ if (nr_entries == ARRAY_SIZE(params->e820_map)) {
+ u32 need = (nr_desc - i) * sizeof(struct e820entry) +
+ sizeof(struct setup_data);
+
+ if (!e820ext || e820ext_size < need)
+ return EFI_BUFFER_TOO_SMALL;
+
+ /* boot_params map full, switch to e820 extended */
+ e820_map = (struct e820entry *)e820ext->data;
+ }
+
+ e820_map->addr = d->phys_addr;
+ e820_map->size = d->num_pages << PAGE_SHIFT;
+ e820_map->type = e820_type;
+ prev = e820_map++;
+ nr_entries++;
}
- boot_params->e820_entries = nr_entries;
+ if (nr_entries > ARRAY_SIZE(params->e820_map)) {
+ u32 nr_e820ext = nr_entries - ARRAY_SIZE(params->e820_map);
+
+ add_e820ext(params, e820ext, nr_e820ext);
+ nr_entries -= nr_e820ext;
+ }
+
+ params->e820_entries = (u8)nr_entries;
+
+ return EFI_SUCCESS;
+}
+
+static efi_status_t alloc_e820ext(u32 nr_desc, struct setup_data **e820ext,
+ u32 *e820ext_size)
+{
+ efi_status_t status;
+ unsigned long size;
+
+ size = sizeof(struct setup_data) +
+ sizeof(struct e820entry) * nr_desc;
+
+ if (*e820ext) {
+ efi_call_phys1(sys_table->boottime->free_pool, *e820ext);
+ *e820ext = NULL;
+ *e820ext_size = 0;
+ }
+
+ status = efi_call_phys3(sys_table->boottime->allocate_pool,
+ EFI_LOADER_DATA, size, e820ext);
+
+ if (status == EFI_SUCCESS)
+ *e820ext_size = size;
+
+ return status;
+}
+
+static efi_status_t exit_boot(struct boot_params *boot_params,
+ void *handle)
+{
+ struct efi_info *efi = &boot_params->efi_info;
+ unsigned long map_sz, key, desc_size;
+ efi_memory_desc_t *mem_map;
+ struct setup_data *e820ext;
+ __u32 e820ext_size;
+ __u32 nr_desc, prev_nr_desc;
+ efi_status_t status;
+ __u32 desc_version;
+ bool called_exit = false;
+ u8 nr_entries;
+ int i;
+
+ nr_desc = 0;
+ e820ext = NULL;
+ e820ext_size = 0;
+
+get_map:
+ status = efi_get_memory_map(sys_table, &mem_map, &map_sz, &desc_size,
+ &desc_version, &key);
+
+ if (status != EFI_SUCCESS)
+ return status;
+
+ prev_nr_desc = nr_desc;
+ nr_desc = map_sz / desc_size;
+ if (nr_desc > prev_nr_desc &&
+ nr_desc > ARRAY_SIZE(boot_params->e820_map)) {
+ u32 nr_e820ext = nr_desc - ARRAY_SIZE(boot_params->e820_map);
+
+ status = alloc_e820ext(nr_e820ext, &e820ext, &e820ext_size);
+ if (status != EFI_SUCCESS)
+ goto free_mem_map;
+
+ efi_call_phys1(sys_table->boottime->free_pool, mem_map);
+ goto get_map; /* Allocated memory, get map again */
+ }
+
+ memcpy(&efi->efi_loader_signature, EFI_LOADER_SIGNATURE, sizeof(__u32));
+ efi->efi_systab = (unsigned long)sys_table;
+ efi->efi_memdesc_size = desc_size;
+ efi->efi_memdesc_version = desc_version;
+ efi->efi_memmap = (unsigned long)mem_map;
+ efi->efi_memmap_size = map_sz;
+
+#ifdef CONFIG_X86_64
+ efi->efi_systab_hi = (unsigned long)sys_table >> 32;
+ efi->efi_memmap_hi = (unsigned long)mem_map >> 32;
+#endif
+
+ /* Might as well exit boot services now */
+ status = efi_call_phys2(sys_table->boottime->exit_boot_services,
+ handle, key);
+ if (status != EFI_SUCCESS) {
+ /*
+ * ExitBootServices() will fail if any of the event
+ * handlers change the memory map. In which case, we
+ * must be prepared to retry, but only once so that
+ * we're guaranteed to exit on repeated failures instead
+ * of spinning forever.
+ */
+ if (called_exit)
+ goto free_mem_map;
+
+ called_exit = true;
+ efi_call_phys1(sys_table->boottime->free_pool, mem_map);
+ goto get_map;
+ }
+
+ /* Historic? */
+ boot_params->alt_mem_k = 32 * 1024;
+
+ status = setup_e820(boot_params, e820ext, e820ext_size);
return EFI_SUCCESS;
--
1.8.1.4
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv3] x86: EFI stub support for large memory maps
2013-09-25 12:58 ` Matt Fleming
@ 2013-09-25 22:45 ` Linn Crosetto
2013-09-26 11:34 ` Matt Fleming
0 siblings, 1 reply; 11+ messages in thread
From: Linn Crosetto @ 2013-09-25 22:45 UTC (permalink / raw)
To: Matt Fleming
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Wed, Sep 25, 2013 at 01:58:40PM +0100, Matt Fleming wrote:
> On Sun, 22 Sep, at 07:59:08PM, Linn Crosetto wrote:
> > This patch fixes a problem with EFI memory maps larger than 128 entries
> > when booting using the EFI stub, which results in overflowing e820_map
> > in boot_params and an eventual halt when checking the map size in
> > sanitize_e820_map().
> >
> > If the number of map entries is greater than what can fit in e820_map,
> > add the extra entries to the setup_data list using type SETUP_E820_EXT.
> > These extra entries are then picked up when the setup_data list is
> > parsed in parse_e820_ext().
> >
> > Signed-off-by: Linn Crosetto <linn@hp.com>
> > ---
> > Changes from v2:
> > * Removed unnecessary optimization in alloc_e820ext() (Matt Fleming)
> > * Fixed a bug where an incorrect buffer size may be passed to
> > get_memory_map when jumping to get_map
> >
> > arch/x86/boot/compressed/eboot.c | 239 +++++++++++++++++++++++++++------------
> > 1 file changed, 167 insertions(+), 72 deletions(-)
>
> Thanks Linn. I applied this to the 'next' branch at,
>
> git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
>
> but it required a bit of massaging to apply on top of the changes
> already there. Could you confirm that my changes are OK? I've included
> the modified commit below.
I have tested the 'next' branch on a system with a large number of entries in
the memory map and the merge appears to be functionally correct.
With the change in commit ae8e9060, I noticed the memory map is no longer placed
in memory allocated with low_alloc(). I have not looked into what effect it
could have, if any.
> + /* Historic? */
> + boot_params->alt_mem_k = 32 * 1024;
> +
> + status = setup_e820(boot_params, e820ext, e820ext_size);
>
> return EFI_SUCCESS;
I might add the following to your merge for semantic reasons:
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 04b228d..a7677ba 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -730,6 +730,8 @@ get_map:
boot_params->alt_mem_k = 32 * 1024;
status = setup_e820(boot_params, e820ext, e820ext_size);
+ if (status != EFI_SUCCESS)
+ return status;
return EFI_SUCCESS;
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv3] x86: EFI stub support for large memory maps
2013-09-25 22:45 ` Linn Crosetto
@ 2013-09-26 11:34 ` Matt Fleming
2013-09-27 23:46 ` Linn Crosetto
0 siblings, 1 reply; 11+ messages in thread
From: Matt Fleming @ 2013-09-26 11:34 UTC (permalink / raw)
To: Linn Crosetto
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Wed, 25 Sep, at 04:45:41PM, Linn Crosetto wrote:
> On Wed, Sep 25, 2013 at 01:58:40PM +0100, Matt Fleming wrote:
> > On Sun, 22 Sep, at 07:59:08PM, Linn Crosetto wrote:
> > > This patch fixes a problem with EFI memory maps larger than 128 entries
> > > when booting using the EFI stub, which results in overflowing e820_map
> > > in boot_params and an eventual halt when checking the map size in
> > > sanitize_e820_map().
> > >
> > > If the number of map entries is greater than what can fit in e820_map,
> > > add the extra entries to the setup_data list using type SETUP_E820_EXT.
> > > These extra entries are then picked up when the setup_data list is
> > > parsed in parse_e820_ext().
> > >
> > > Signed-off-by: Linn Crosetto <linn@hp.com>
> > > ---
> > > Changes from v2:
> > > * Removed unnecessary optimization in alloc_e820ext() (Matt Fleming)
> > > * Fixed a bug where an incorrect buffer size may be passed to
> > > get_memory_map when jumping to get_map
> > >
> > > arch/x86/boot/compressed/eboot.c | 239 +++++++++++++++++++++++++++------------
> > > 1 file changed, 167 insertions(+), 72 deletions(-)
> >
> > Thanks Linn. I applied this to the 'next' branch at,
> >
> > git://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git
> >
> > but it required a bit of massaging to apply on top of the changes
> > already there. Could you confirm that my changes are OK? I've included
> > the modified commit below.
>
> I have tested the 'next' branch on a system with a large number of entries in
> the memory map and the merge appears to be functionally correct.
Excellent, thank you for verifying.
> With the change in commit ae8e9060, I noticed the memory map is no longer placed
> in memory allocated with low_alloc(). I have not looked into what effect it
> could have, if any.
Correct. I haven't run into any problems on my test machines.
> > + /* Historic? */
> > + boot_params->alt_mem_k = 32 * 1024;
> > +
> > + status = setup_e820(boot_params, e820ext, e820ext_size);
> >
> > return EFI_SUCCESS;
>
> I might add the following to your merge for semantic reasons:
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 04b228d..a7677ba 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -730,6 +730,8 @@ get_map:
> boot_params->alt_mem_k = 32 * 1024;
>
> status = setup_e820(boot_params, e820ext, e820ext_size);
> + if (status != EFI_SUCCESS)
> + return status;
>
> return EFI_SUCCESS;
Aha, nice catch! Though if setup_e820() fails we should be jumping to
the 'free_mem_map' label so we don't leak the memory map, like so,
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index 04b228d..602950b 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -730,8 +730,8 @@ get_map:
boot_params->alt_mem_k = 32 * 1024;
status = setup_e820(boot_params, e820ext, e820ext_size);
-
- return EFI_SUCCESS;
+ if (status == EFI_SUCCESS)
+ return status;
free_mem_map:
efi_call_phys1(sys_table->boottime->free_pool, mem_map);
I've fixed this up and pushed out a new patch.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCHv3] x86: EFI stub support for large memory maps
2013-09-26 11:34 ` Matt Fleming
@ 2013-09-27 23:46 ` Linn Crosetto
2013-09-30 9:22 ` Matt Fleming
0 siblings, 1 reply; 11+ messages in thread
From: Linn Crosetto @ 2013-09-27 23:46 UTC (permalink / raw)
To: Matt Fleming
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Thu, Sep 26, 2013 at 12:34:00PM +0100, Matt Fleming wrote:
> > I might add the following to your merge for semantic reasons:
> >
> > diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> > index 04b228d..a7677ba 100644
> > --- a/arch/x86/boot/compressed/eboot.c
> > +++ b/arch/x86/boot/compressed/eboot.c
> > @@ -730,6 +730,8 @@ get_map:
> > boot_params->alt_mem_k = 32 * 1024;
> >
> > status = setup_e820(boot_params, e820ext, e820ext_size);
> > + if (status != EFI_SUCCESS)
> > + return status;
> >
> > return EFI_SUCCESS;
>
> Aha, nice catch! Though if setup_e820() fails we should be jumping to
> the 'free_mem_map' label so we don't leak the memory map, like so,
>
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index 04b228d..602950b 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -730,8 +730,8 @@ get_map:
> boot_params->alt_mem_k = 32 * 1024;
>
> status = setup_e820(boot_params, e820ext, e820ext_size);
> -
> - return EFI_SUCCESS;
> + if (status == EFI_SUCCESS)
> + return status;
>
> free_mem_map:
> efi_call_phys1(sys_table->boottime->free_pool, mem_map);
Given that we have already successfully called exit_boot_services, can we still
make this call to free_pool?
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCHv3] x86: EFI stub support for large memory maps
2013-09-27 23:46 ` Linn Crosetto
@ 2013-09-30 9:22 ` Matt Fleming
0 siblings, 0 replies; 11+ messages in thread
From: Matt Fleming @ 2013-09-30 9:22 UTC (permalink / raw)
To: Linn Crosetto
Cc: matt.fleming, hpa, tglx, mingo, x86, yinghai, linux-efi, linux-kernel
On Fri, 27 Sep, at 05:46:07PM, Linn Crosetto wrote:
> Given that we have already successfully called exit_boot_services, can
> we still make this call to free_pool?
Urgh, good point. The answer is "no, not really".
I'll add your change verbatim and push out the 'next' branch again.
--
Matt Fleming, Intel Open Source Technology Center
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2013-09-30 9:22 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-23 21:02 [PATCH] x86: EFI stub support for large memory maps Linn Crosetto
2013-09-06 10:03 ` Matt Fleming
2013-09-06 17:04 ` [PATCHv2] " Linn Crosetto
2013-09-17 20:14 ` Matt Fleming
2013-09-23 0:23 ` Linn Crosetto
2013-09-23 1:59 ` [PATCHv3] " Linn Crosetto
2013-09-25 12:58 ` Matt Fleming
2013-09-25 22:45 ` Linn Crosetto
2013-09-26 11:34 ` Matt Fleming
2013-09-27 23:46 ` Linn Crosetto
2013-09-30 9:22 ` Matt Fleming
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).