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