Re: [PATCH v2 4/4] x86: Pass memory range via E820 for kdump

[WANG Chao <chaowang@redhat.com>]

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

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec