On 02/27/14 at 01:53pm, Linn Crosetto wrote:
> Hi Chao,
>
> On Thu, Feb 20, 2014 at 02:28:32AM -0700, WANG Chao wrote:
>
> [..]
>
> > +static void setup_e820_ext(struct kexec_info *info, struct x86_linux_param_header *real_mode,
> > + struct memory_range *range, int nr_range)
> > +{
> > + struct setup_data *sd;
> > + struct e820entry *e820;
> > + int i, j, nr_range_ext;
> > +
> > + nr_range_ext = nr_range - E820MAX;
> > + sd = malloc(sizeof(struct setup_data) + nr_range_ext * sizeof(struct e820entry));
> > + sd->next = 0;
> > + sd->len = nr_range_ext * sizeof(struct e820entry);
> > + sd->type = SETUP_E820_EXT;
> > +
> > + e820 = (struct e820entry *) sd->data;
> > + dbgprintf("Extended E820 via setup_data:\n");
> > + for(i = 0, j = E820MAX; i < nr_range_ext; i++, j++) {
> > + e820[i].addr = range[j].start;
> > + e820[i].size = range[j].end - range[j].start;
> > + switch (range[j].type) {
> > + case RANGE_RAM:
> > + e820[i].type = E820_RAM;
> > + break;
> > + case RANGE_ACPI:
> > + e820[i].type = E820_ACPI;
> > + break;
> > + case RANGE_ACPI_NVS:
> > + e820[i].type = E820_NVS;
> > + break;
> > + default:
> > + case RANGE_RESERVED:
> > + e820[i].type = E820_RESERVED;
> > + break;
> > + }
> > + dbgprintf("%016lx-%016lx (%d)\n",
> > + e820[i].addr,
> > + e820[i].addr + e820[i].size - 1,
> > + e820[i].type);
> > +
> > + if (range[j].type != RANGE_RAM)
> > + continue;
> > + if ((range[j].start <= 0x100000) && range[j].end > 0x100000) {
> > + unsigned long long mem_k = (range[j].end >> 10) - (0x100000 >> 10);
> > + real_mode->ext_mem_k = mem_k;
> > + real_mode->alt_mem_k = mem_k;
> > + if (mem_k > 0xfc00) {
> > + real_mode->ext_mem_k = 0xfc00; /* 64M */
> > + }
> > + if (mem_k > 0xffffffff) {
> > + real_mode->alt_mem_k = 0xffffffff;
> > + }
> > + }
> > + }
> > + add_setup_data(info, real_mode, sd);
> > + free(sd);
>
> Remove this call to free(sd) or the kernel will not boot if there are more than
> E820MAX entries.
You're right.
>
> > +}
> > +
> > +static void setup_e820(struct kexec_info *info, struct x86_linux_param_header *real_mode,
> > + struct memory_range *range, int nr_range)
> > +{
> > +
> > + int nr_range_saved = nr_range;
> > + int i;
> > +
> > + if (nr_range > E820MAX) {
> > + nr_range = E820MAX;
> > + }
> > +
> > + real_mode->e820_map_nr = nr_range;
> > + dbgprintf("E820 memmap:\n");
> > + for(i = 0; i < nr_range; i++) {
> > + real_mode->e820_map[i].addr = range[i].start;
> > + real_mode->e820_map[i].size = range[i].end - range[i].start;
> > + switch (range[i].type) {
> > + case RANGE_RAM:
> > + real_mode->e820_map[i].type = E820_RAM;
> > + break;
> > + case RANGE_ACPI:
> > + real_mode->e820_map[i].type = E820_ACPI;
> > + break;
> > + case RANGE_ACPI_NVS:
> > + real_mode->e820_map[i].type = E820_NVS;
> > + break;
> > + default:
> > + case RANGE_RESERVED:
> > + real_mode->e820_map[i].type = E820_RESERVED;
> > + break;
> > + }
> > + dbgprintf("%016lx-%016lx (%d)\n",
> > + real_mode->e820_map[i].addr,
> > + real_mode->e820_map[i].addr + real_mode->e820_map[i].size - 1,
> > + real_mode->e820_map[i].type);
> > +
> > + if (range[i].type != RANGE_RAM)
> > + continue;
> > + if ((range[i].start <= 0x100000) && range[i].end > 0x100000) {
> > + unsigned long long mem_k = (range[i].end >> 10) - (0x100000 >> 10);
> > + real_mode->ext_mem_k = mem_k;
> > + real_mode->alt_mem_k = mem_k;
> > + if (mem_k > 0xfc00) {
> > + real_mode->ext_mem_k = 0xfc00; /* 64M */
> > + }
> > + if (mem_k > 0xffffffff) {
> > + real_mode->alt_mem_k = 0xffffffff;
> > + }
> > + }
> > + }
> > +
> > + if (nr_range_saved > E820MAX) {
> > + dbgprintf("extra E820 memmap are passed via setup_data\n");
> > + setup_e820_ext(info, real_mode, range, nr_range_saved);
> > + }
> > +}
> > +
>
> setup_e820() and setup_e820_ext() contain a lot of duplication. You could
> combine them into a single function, similar to the implementation of
> setup_e820() in the kernel.
OK. That makes sense to me.
>
> > static int
> > get_efi_mem_desc_version(struct x86_linux_param_header *real_mode)
> > {
> > @@ -704,7 +830,7 @@ void setup_linux_system_parameters(struct kexec_info *info,
>
> [..]
>
> > + if (info->kexec_flags & KEXEC_ON_CRASH && !pass_memmap_cmdline) {
> > + range = crash_memory_range;
> > + ranges = crash_memory_ranges;
> > + } else {
> > + range = info->memory_range;
> > + ranges = info->memory_ranges;
> > }
>
> You can move this code into setup_e820(), since range and ranges are not used
> elsewhere in setup_linux_system_parameters().
Will do.
>
> > + setup_e820(info, real_mode, range, ranges);
> >
> > /* fill the EDD information */
> > setup_edd_info(real_mode);
> > --
> > 1.8.5.3
>
> I tested boot (with the above fix) on a system with a large memory map, and can
> easily test changes, if needed.
Thanks for testing. The result on that real system is helpful and convincing.
I'm holding off v3 for serveral days in case someone would have a
comment.
Thanks
WANG Chao
