Re: [PATCHv2] x86/boot/KASLR: skip the specified crashkernel reserved region

[Baoquan He]

On 03/29/19 at 06:00pm, Pingfan Liu wrote:
> On Fri, Mar 29, 2019 at 3:34 PM Baoquan He <bhe@xxxxxxxxxx> wrote:
> >
> > On 03/29/19 at 03:25pm, Pingfan Liu wrote:
> > > On Fri, Mar 29, 2019 at 2:27 PM Baoquan He <bhe@xxxxxxxxxx> wrote:
> > > >
> > > > On 03/29/19 at 01:45pm, Pingfan Liu wrote:
> > > > > On Fri, Mar 22, 2019 at 4:34 PM Baoquan He <bhe@xxxxxxxxxx> wrote:
> > > > > >
> > > > > > On 03/22/19 at 03:52pm, Baoquan He wrote:
> > > > > > > On 03/22/19 at 03:43pm, Pingfan Liu wrote:
> > > > > > > > > > +/* parse crashkernel=x@y option */
> > > > > > > > > > +static void mem_avoid_crashkernel_simple(char *option)
> > > > > > > > >
> > > > > > > > > Chao ever mentioned this, I want to ask again, why does it has to be
> > > > > > > > > xxx_simple()?
> > > > > > > > >
> > > > > > > > Seems that I had replied Chao's question in another email. The naming
> > > > > > > > follows the function parse_crashkernel_simple(), as the notes above
> > > > > > >                        ~~~~~~~~~~~~~~~~~~~~~~~~
> > > > > > >
> > > > > > > Sorry, I don't get.  typo?
> > > > > >
> > > > > > OK, I misunderstood it. We do have parse_crashkernel_simple() to handle
> > > > > > crashkernel=size[@offset] case, to differente with other complicated
> > > > > > cases, like crashkernel=size,[high|low],
> > > > > >
> > > > > > Then I am fine with this naming. Soryy about the noise.
> > > > > >
> > > > > > By the way, do you think if we should take care of this case:
> > > > > > crashkernel=<range1>:<size1>[,<range2>:<size2>,...][@offset]
> > > > > >
> > > > > > It can also specify @offset. Not sure if it's too complicated, you may
> > > > > > have a investigation.
> > > > > >
> > > > > In this case, kernel should get the total memory size info. So
> > > > > process_e820_entries() or process_efi_entries() should be called
> > > > > twice. One before handle_mem_options(), so crashkernel can evaluate
> > > > > the reserved size. It is doable, and what is your opinion about the
> > > >
> > > > You mean calling process_e820_entries to calculate the RAM size in
> > > > system? I may not do like that, please check what __find_max_addr() is
> > > > doing. Did I get it?
> > >
> > > Yes, you got my meaning. But __find_max_addr() relies on the info, fed
> > > by e820__memblock_setup(). It also involves the iteration of all e820
> > > entries to get the max address. No essential difference, right?
> >
> > Hmm, I would say iterating e820 or efi entries to get the mas addr should be
> > different with calling process_e820_entries(). The 1st is much simpler,
> > right?
> >
> Yes. My original meaning is to reuse process_e820_entries(), but does
> not call process_mem_region() at the first time.

Got it. That would be nice, but it will bring hackery which Thomas and
Boris have clearly expressed disliking. Adding a new function to find
the max addr, it truly doesn't reuse code, while it makes the code paths
and logic are pretty clear. These avoiding cases are bloating code,
however they usually don't happen in a same system. We make a simply and
clear logic to make code strightforward with good code comments and
printing message, it will be easier to debug issue if happened.

So, I think a draft patch is welcomed, we can have a look at the logic,
then polish it later to make a formal patch. Makes sense?

Thanks
Baoquan