Messages in this thread | | | Date | Mon, 27 Aug 2018 14:31:35 +0800 | From | Chao Fan <> | Subject | Re: [PATCH v5 4/4] x86/boot/KASLR: Limit kaslr to choosing the immovable memory |
| |
On Mon, Aug 27, 2018 at 02:28:54PM +0800, Chao Fan wrote: >On Mon, Aug 27, 2018 at 01:56:07PM +0800, Baoquan He wrote: >>On 08/07/18 at 02:50pm, Chao Fan wrote: [...] >> >>Is it possible to take num_immovable_mem definition out from #ifdef >>CONFIG_MEMORY_HOTREMOVE block and check it here like below? This way, >>one level of indentation can be reduced in the for loop, and code is >>more readable. >> > >I think there is a mistake. > >The logical is: >if (#ifdef CONFIG_MEMORY_HOTREMOVE) && (num_immovable_mem > 0) > then A; >else > then B; > >But below is:
Sorry for that, here the 'below' means the code in your mail.
Thanks, Chao Fan
>if (num_immovable_mem > 0) > then B; >else if (#ifdef CONFIG_MEMORY_HOTREMOVE) > then A; >else > nothing; > >The precondition of the loop is (num_immovable_mem > 0), because >there is only one condition that we need go the A code: >CONFIG_MEMORY_HOTREMOVE is defined, and memory information in srat >found. > >But there is many conditions we go the B code: >1. CONFIG_MEMORY_HOTREMOVE is not defined. >2. CONFIG_MEMORY_HOTREMOVE defined, but we didn't get the right acpi tables >3. CONFIG_MEMORY_HOTREMOVE defined, or there is only one node in this machine. > >Yes, the code is hard to read, but you have changed the logical, there >is a compromise method, I don't know whether is better: > >#ifdef CONFIG_MEMORY_HOTREMOVE > if (num_immovable_mem == 0) > goto B; > > for (i = 0; i < num_immovable_mem; i++) { > ... > } >#endif > >B: > slots_count(region, minimum, image_size); > > if (slot_area_index == MAX_SLOT_AREA) { > debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n"); > return 1; > } > return 0; > > >> >>static bool process_mem_region(struct mem_vector *region, >> unsigned long long minimum, >> unsigned long long image_size) >>{ >> >> /* >> * If no immovable memory found, or MEMORY_HOTREMOVE disabled, >> * walk all the regions, so use region directely. >> */ >> if (num_immovable_mem > 0) { >> slots_count(region, minimum, image_size); >> >> if (slot_area_index == MAX_SLOT_AREA) { >> debug_putstr("Aborted e820/efi memmap scan (slot_areas full)!\n"); >> return 1; >> } >> return 0; >> } >> >>#ifdef CONFIG_MEMORY_HOTREMOVE >> for (i = 0; i < num_immovable_mem; i++) { >> ... >> } >>#endif >>} >>
| |