[PATCH v4 2/4] x86: Store memory ranges globally used for crash kernel to boot into

[WANG Chao]

On 04/01/14 at 05:36pm, Dave Young wrote:
> On 04/01/14 at 04:54pm, WANG Chao wrote:
> > On 04/01/14 at 04:41pm, Dave Young wrote:
> > > On 04/01/14 at 03:04pm, WANG Chao wrote:
> > > > On 03/28/14 at 02:43pm, Dave Young wrote:
> > > > > On 03/28/14 at 02:13pm, WANG Chao wrote:
> > > > > > On 03/28/14 at 11:24am, Dave Young wrote:
> > > > > > > >  
> > > > > > > > +static void exclude_ram(struct memory_range *mr, int *nr_mr)
> > > > > > > > +{
> > > > > > > > +	int ranges, i, j, m;
> > > > > > > > +
> > > > > > > > +	ranges = *nr_mr;
> > > > > > > > +	for (i = 0, j = 0; i < ranges; i++) {
> > > > > > > > +		if (mr[j].type == RANGE_RAM) {
> > > > > > > > +			dbgprintf("Remove RAM %016llx-%016llxx: (%d)\n", mr[j].start, mr[j].end, mr[j].type);
> > > > > > > > +			for (m = j; m < *nr_mr; m++)
> > > > > > > > +				mr[m] = mr[m+1];
> > > > > > > > +			(*nr_mr)--;
> > > > > > > > +		} else {
> > > > > > > > +			j++;
> > > > > > > > +		}
> > > > > > > > +	}
> > > > > > > > +
> > > > > > > > +	dbgprint_mem_range("After remove RAM", mr, *nr_mr);
> > > > > > > > +}
> > > > > > > 
> > > > > > > This is probably not necessary, what I understand you are doing is below:
> > > > > > > 
> > > > > > > get_crash_memory_ranges()
> > > > > > >  -> collect all SYSTEM_RAM, ACPI, ACPI_NVS ranges, exclude crash reserved ranges.
> > > > > > >  -> the system ram ranges are used to create elf header
> > > > > > >  -> the ACPI, ACPI_NVS ranges are used by cmdline_add_memmap_acpi etc.
> > > > > > 
> > > > > > Yes.
> > > > > > 
> > > > > > > 
> > > > > > > memmap_p
> > > > > > >  -> contains all the crash reserved ranges
> > > > > > >  -> to be used by cmdline_add_memmap
> > > > > > 
> > > > > > There's no memmap_p. I'll reuse crash_memory_ranges structure to store
> > > > > > crash reserved ranges, ACPI and ACPI_NVS ranges. So after building ELF
> > > > > > headers for 1st kernel memory ranges, all I have to do is exclude the
> > > > > > SYSTEM_RAM and add crash_reserved to crash_memory_ranges. And then
> > > > > > crash_memory_ranges can be used as 2nd kernel memory ranges.
> > > > > 
> > > > > How about do nothing and directly use the mem_ranges:
> > > > >  * skip RANGE_RAM, only add the range which is not RANGE_RAM
> > > > >  * if the range is RANGE_CRASH_KERNEL (introduce a new type?) then use it as SYSTEM_RAM for 2nd kernel.
> > > > 
> > > > I prefer not do this change in this patchset. Since current
> > > > implementation is fine, it looks more like a cleanup to me and we can do
> > > > that later.
> > > 
> > > Ok, that's fine, but I'm still not keen about exclude_ram. Could you manage
> > > to drop this function? 
> > 
> > exclude_ram() is just a static function that my code would use and I
> > certainly need a function like that this time. So why are you still keen
> > about dropping it :(
> 
> Just suppose you agree with the future cleanup, you introduce it now and remove it
> later it looks not good so I wonder if it's possible to drop it now and check the
> range type instead.

The cleanup is another topic. What to clean up and how to clean up is
yet to be determined.

I personally agree with cleanup in the future, but it still depends on
many things and I can't make any promise that exclude_ram can be removed
in the future.

> 
> If it's a *must* then I will not object

OK. I'll use it, unless I figure out something else ...

Thanks for your suggestions!
WANG Chao