On 01/07/2016 at 10:36 AM, Minfei Huang wrote:
> On 01/07/16 at 10:14am, Xunlei Pang wrote:
>>>> +static int
>>>> +kexec_mark_range(unsigned long start, unsigned long end, bool protect)
>>>> +{
>>>> + struct page *page;
>>>> + unsigned int nr_pages;
>>>> +
>>>> + /* For physical range: [start, end] */
>>>> + if (!start || !end || start > end)
>>>> + return 0;
>>> Hi, Xunlei.
>>>
>>> if (start > end)
>>> return 0;
>> If both start and end are zero, we want to return directly, so the two
>> more check doesn't hurt.
> How about if the start is equal to 0, and end is larger than 0? It is
> better to make code more robust, although it never happen in currect
> kexec code.
Hmm, this will be better:
if (!end || start > end)
return 0;
it handles the common case not using crash_low_res(start and end are 0).
Regards,
Xunlei
>
>>> See the below comment.
>>>> +
>>>> + page = pfn_to_page(start >> PAGE_SHIFT);
>>>> + nr_pages = (end + PAGE_SIZE - start) >> PAGE_SHIFT;
>>> As I commented in last version, it is better to cover the case if the
>>> range from start to end acrosses two pages.
>> right.
>>
>>>> + if (protect)
>>>> + return set_pages_ro(page, nr_pages);
>>>> + else
>>>> + return set_pages_rw(page, nr_pages);
>>>> +}
>>>> +
>>>> +static void kexec_mark_crashkres(bool protect)
>>>> +{
>>>> + unsigned long control;
>>>> +
>>>> + kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
>>> Adding the following if test to test crashk_low_res is better. Then we
>>> do not need to test if start or end is equal to 0 in kexec_mark_range.
>>>
>>> if (crashk_low_res.start != crashk_low_res.end) {
>>> kexec_mark_range(crashk_low_res.start,
>>> crashk_low_res.end, protect);
>>> }
>> The checks in kexec_mark_range() will handle the case, it's not
>> performance-critical path and will make the code less clean.
>>
>>>> +
>>>> + /* Don't touch the control code page used in crash_kexec().*/
>>>> + control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
>>>> + /* Control code page is located in the 2nd page. */
>>>> + kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
>>>> + kexec_mark_range(control + 2 * PAGE_SIZE, crashk_res.end, protect);
>>> I think it is more readable, if we use MACRO KEXEC_CONTROL_PAGE_SIZE,
>>> instead of using 2*PAGE_SIZE directly.
>> OK.
>>
>> How about the following update:
>> +static void kexec_mark_crashkres(bool protect)
>> +{
>> + unsigned long control;
>> +
>> + kexec_mark_range(crashk_low_res.start, crashk_low_res.end, protect);
>> +
>> + /* Don't touch the control code page used in crash_kexec().*/
>> + control = PFN_PHYS(page_to_pfn(kexec_crash_image->control_code_page));
>> + /* Control code page is located in the 2nd page. */
>> + kexec_mark_range(crashk_res.start, control + PAGE_SIZE - 1, protect);
>> + control += KEXEC_CONTROL_PAGE_SIZE;
>> + kexec_mark_range(control, crashk_res.end, protect);
>> +}
> I'm fine with this.
>
> Thanks
> Minfei
>
> _______________________________________________
> kexec mailing list
> kexec at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/kexec
