On Sun, 7 Jan 2018, Borislav Petkov wrote:
On Sun, Jan 07, 2018 at 06:33:17PM +0800, Jike Song wrote:
Look at one of the code snippets:
162 if (pgd_none(*pgd)) { 163 unsigned long new_p4d_page = __get_free_page(gfp); 164 if (!new_p4d_page) 165 return NULL; 166 167 if (pgd_none(*pgd)) { 168 set_pgd(pgd, __pgd(_KERNPG_TABLE | __pa(new_p4d_page))); 169 new_p4d_page = 0; 170 } 171 if (new_p4d_page) 172 free_page(new_p4d_page); 173 }There can't be any difference between two pgd_none(*pgd) at L162 and L167, so it's always false at L171.
I think this is a remnant from the kaiser version which did this:
if (pud_none(*pud)) { unsigned long new_pmd_page = __get_free_page(gfp); if (!new_pmd_page) return NULL; spin_lock(&shadow_table_allocation_lock); if (pud_none(*pud)) set_pud(pud, __pud(_KERNPG_TABLE | __pa(new_pmd_page))); else free_page(new_pmd_page); spin_unlock(&shadow_table_allocation_lock); }I was wondering too, why the duplicated checks.
Which has this explanation about the need for the locking:
/*
- At runtime, the only things we map are some things for CPU
- hotplug, and stacks for new processes. No two CPUs will ever
- be populating the same addresses, so we only need to ensure
- that we protect between two CPUs trying to allocate and
- populate the same page table page.
- Only take this lock when doing a set_p[4um]d(), but it is not
- needed for doing a set_pte(). We assume that only the *owner*
- of a given allocation will be doing this for _their_
- allocation.
- This ensures that once a system has been running for a while
- and there have been stacks all over and these page tables
- are fully populated, there will be no further acquisitions of
- this lock.
*/ static DEFINE_SPINLOCK(shadow_table_allocation_lock);
Now I have my suspicions why that's not needed anymore upstream but I'd let tglx explain better.
We got rid of all that runtime mapping stuff and the functions are only called from pti_init(). So the locking and therefor the tests above are not needed anymore. While at it we should mark all those function __init.
Thanks,
tglx