Hi Thomas, Here is some feedback on the KPTI patchset. Sorry for not replying to the patch, I was not CC'd on the original email, and don't have it in my inbox.
I notice that fill_ldt() sets the desc->type with "|= 1", whereas all other operations on the desc type are done with a type enum based on clearly defined bits. Is the hardcoded "1" on purpose ? arch/x86/include/asm/processor.h: "+ * With page table isolation enabled, we map the LDT in ... [stay tuned]" I look forward to publication of the next chapter containing the rest of this sentence. When is it due ? ;) arch/x86/include/asm/tlbflush.h: @@ -135,6 +193,24 @@ struct tlb_state { bool invalidate_other; ^ I thought using bool in structures was discouraged ? Arguably, there are other booleans in that structure already. +static void free_ldt_pgtables(struct mm_struct *mm) +{ +#ifdef CONFIG_PAGE_TABLE_ISOLATION + struct mmu_gather tlb; + unsigned long start = LDT_BASE_ADDR; + unsigned long end = start + (1UL << PGDIR_SHIFT); + + if (!static_cpu_has(X86_FEATURE_PTI)) + return; + + tlb_gather_mmu(&tlb, mm, start, end); + free_pgd_range(&tlb, start, end, start, end); + tlb_finish_mmu(&tlb, start, end); +#endif ^ AFAIK, the usual approach is to move the #ifdef outside of the function body, and have one empty function. @@ -156,6 +271,12 @@ int ldt_dup_context(struct mm_struct *old_mm, struct mm_struct *mm) new_ldt->nr_entries * LDT_ENTRY_SIZE); finalize_ldt_struct(new_ldt); + retval = map_ldt_struct(mm, new_ldt, 0); + if (retval) { + free_ldt_pgtables(mm); + free_ldt_struct(new_ldt); + goto out_unlock; + } mm->context.ldt = new_ldt; out_unlock: ^ I don't get why it does "free_ldt_pgtables(mm)" on the mm argument, but it's not done in other error paths. Perhaps it's OK, but ownership seems non-obvious. @@ -287,6 +413,18 @@ static int write_ldt(void __user *ptr, unsigned long bytecount, int oldmode) new_ldt->entries[ldt_info.entry_number] = ldt; finalize_ldt_struct(new_ldt); + /* + * If we are using PTI, map the new LDT into the userspace pagetables. + * If there is already an LDT, use the other slot so that other CPUs + * will continue to use the old LDT until install_ldt() switches + * them over to the new LDT. + */ + error = map_ldt_struct(mm, new_ldt, old_ldt ? !old_ldt->slot : 0); + if (error) { + free_ldt_struct(old_ldt); + goto out_unlock; + } + ^ is it really "old_ldt" that we want freed on error here ? Or should it be "new_ldt" ? --- a/arch/x86/mm/cpu_entry_area.c +++ b/arch/x86/mm/cpu_entry_area.c @@ -38,6 +38,32 @@ cea_map_percpu_pages(void *cea_vaddr, void *ptr, int pages, pgprot_t prot) cea_set_pte(cea_vaddr, per_cpu_ptr_to_phys(ptr), prot); } +static void percpu_setup_debug_store(int cpu) +{ +#ifdef CONFIG_CPU_SUP_INTEL ^ This ifdef should ideally be outside of the function, defining an empty function in the #else. + /* + * Force the population of PMDs for not yet allocated per cpu + * memory like debug store buffers. + */ + npages = sizeof(struct debug_store_buffers) / PAGE_SIZE; + for (; npages; npages--, cea += PAGE_SIZE) + cea_set_pte(cea, 0, PAGE_NONE); ^ the code above (in percpu_setup_debug_store()) depends on having struct debug_store_buffers's size being a multiple of PAGE_SIZE. A comment should be added near the structure declaration to document this requirement. +static void __init pti_setup_espfix64(void) +{ +#ifdef CONFIG_X86_ESPFIX64 + pti_clone_p4d(ESPFIX_BASE_ADDR); +#endif +} Seeing how this ifdef within function layout is everywhere in the patch, I start to wonder whether I missed a coding style guideline somewhere... ? +/* + * We get here when we do something requiring a TLB invalidation + * but could not go invalidate all of the contexts. We do the + * necessary invalidation by clearing out the 'ctx_id' which + * forces a TLB flush when the context is loaded. + */ +void clear_asid_other(void) +{ + u16 asid; + + /* + * This is only expected to be set if we have disabled + * kernel _PAGE_GLOBAL pages. + */ + if (!static_cpu_has(X86_FEATURE_PTI)) { + WARN_ON_ONCE(1); + return; + } + + for (asid = 0; asid < TLB_NR_DYN_ASIDS; asid++) { + /* Do not need to flush the current asid */ + if (asid == this_cpu_read(cpu_tlbstate.loaded_mm_asid)) + continue; + /* + * Make sure the next time we go to switch to + * this asid, we do a flush: + */ + this_cpu_write(cpu_tlbstate.ctxs[asid].ctx_id, 0); + } + this_cpu_write(cpu_tlbstate.invalidate_other, false); +} Can this be called with preemption enabled ? If so, what happens if migrated ? Thanks, Mathieu -- Mathieu Desnoyers EfficiOS Inc. http://www.efficios.com