Re: [PATCH v2 1/2] x86/boot/64: Make level2_kernel_pgt pages invalid outside kernel area.

[Steve Wahl]

On Mon, Sep 23, 2019 at 02:19:46PM -0700, Dave Hansen wrote:
> On 9/23/19 11:15 AM, Steve Wahl wrote:
> >  	pmd = fixup_pointer(level2_kernel_pgt, physaddr);
> > -	for (i = 0; i < PTRS_PER_PMD; i++) {
> > +	for (i = 0; i < pmd_index((unsigned long)_text); i++)
> > +		pmd[i] &= ~_PAGE_PRESENT;
> > +
> > +	for (; i <= pmd_index((unsigned long)_end); i++)
> >  		if (pmd[i] & _PAGE_PRESENT)
> >  			pmd[i] += load_delta;
> > -	}
> > +
> > +	for (; i < PTRS_PER_PMD; i++)
> > +		pmd[i] &= ~_PAGE_PRESENT;
> 
> This is looking a bunch better.  The broken-up loop could probably use
> some comments, or you could combine it back to a single loop like this:
> 
> 	int text_start_pmd_index = pmd_index((unsigned long)_text);
> 	int text_end_pmd_index   = pmd_index((unsigned long)_end);
> 
> 	for (i = 0; i < PTRS_PER_PMD; i++) {
> 		if ((i < text_start_pmd_index) ||
> 		    (i > text_end_pmd_index)) {
> 			/* Unmap entries not mapping the kernel image */
> 			pmd[i] &= ~_PAGE_PRESENT;
> 		} else if (pmd[i] & _PAGE_PRESENT)
>  			pmd[i] += load_delta;
> 		}
> 	}

That's funny, I wrote it very close to that way initially, and
re-wrote it broken-up loop style because I thought it better conveyed
my intention.  (Mark pages before the kernel image as invalid, fixup
the pages that are in kernel image range, mark pages after the kernel
image as invalid.)

Given that, would you feel better with A) the way I have it, B) your
rewrite, or C) with an inline comment for each part of the loop:

	pmd = fixup_pointer(level2_kernel_pgt, physaddr);

	/* invalidate pages before the kernel image */
	for (i = 0; i < pmd_index((unsigned long)_text); i++)
		pmd[i] &= ~_PAGE_PRESENT;

	/* fixup pages that are part of the kernel image */
	for (; i <= pmd_index((unsigned long)_end); i++)
		if (pmd[i] & _PAGE_PRESENT)
			pmd[i] += load_delta;

	/* invalidate pages after the kernel image */
	for (; i < PTRS_PER_PMD; i++)
		pmd[i] &= ~_PAGE_PRESENT;

> Although I'd prefer it get commented or rewritten, it's passable like
> this, so:
> 
> Reviewed-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx>

Thanks for your input!  

--> Steve Wahl

-- 
Steve Wahl, Hewlett Packard Enterprise