Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1303247

Re: [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres()

Show all headers | View raw

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.

> 
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Back to linux.kernel | Previous | Next — Previous in thread | Next in thread | Find similar | Unroll thread

Thread

[PATCH v2 1/2] kexec: Introduce a protection mechanism for the crashkernel reserved memory Xunlei Pang <xlpang@redhat.com> - 2016-01-06 11:00 +0100
  [PATCH v2 2/2] kexec: Provide arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-06 11:00 +0100
    Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Minfei Huang <mhuang@redhat.com> - 2016-01-06 18:10 +0100
      Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 03:20 +0100
        Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 03:30 +0100
        Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Minfei Huang <mhuang@redhat.com> - 2016-01-07 03:40 +0100
          Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 06:10 +0100
            Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Petr Tesarik <ptesarik@suse.cz> - 2016-01-07 10:30 +0100
              Re: [PATCH v2 2/2] kexec: Provide  arch_kexec_protect(unprotect)_crashkres() Xunlei Pang <xlpang@redhat.com> - 2016-01-07 12:20 +0100

csiph-web