On Mon, Apr 15, 2019 at 08:58:52AM -0700, Dave Hansen wrote:
> On 4/15/19 7:55 AM, Singh, Brijesh wrote:
> > static unsigned long __meminit
> > phys_pte_init(pte_t *pte_page, unsigned long paddr, unsigned long
> > paddr_end,
> > - pgprot_t prot)
> > + pgprot_t prot, bool safe)
> > {
> > unsigned long pages = 0, paddr_next;
> > unsigned long paddr_last = paddr_end;
> > @@ -432,7 +463,7 @@ phys_pte_init(pte_t *pte_page, unsigned long paddr,
> > unsigned long paddr_end,
> > E820_TYPE_RAM) &&
> > !e820__mapped_any(paddr & PAGE_MASK, paddr_next,
> > E820_TYPE_RESERVED_KERN))
> > - set_pte_safe(pte, __pte(0));
> > + __set_pte(pte, __pte(0), safe);
> > continue;
> > }
>
> The changelog is great, btw.
>
> But, I'm not a big fan of propagating the 'safe' nomenclature. Could
> we, at least, call it 'overwrite_safe' or something if we're going to
> have a variable name? Or even, 'new_entries_only' or something that
> actually conveys meaning?
>
> Because, just reading it, I always wonder "why do we have an unsafe
> variant, that's stupid" every time. :)
s/safe/init/ on the whole thing?
And maybe even back on the initial _safe functions? Because all of this
is about initializing page-tables, which is a TLB *safe* operation I
suppose :-)
> > +#define DEFINE_ENTRY(type1, type2, safe) \
> > +static inline void __set_##type1(type1##_t *arg1, \
> > + type2##_t arg2, bool safe) \
> > +{ \
> > + if (safe) \
> > + set_##type1##_safe(arg1, arg2); \
> > + else \
> > + set_##type1(arg1, arg2); \
> > +}
>
> While I appreciate the brevity that these macros allow, I detest their
> ability to thwart cscope and grep. I guess it's just one file, but it
> does make me grumble a bit.
There is scripts/tags.sh where you can add to regex_c to teach
cscope/ctags about magic macros.
> Also, can we do better than "__"? Aren't these specific to
> initialization, and only for the kernel? Maybe we should call them
> meminit_set_pte() or kern_set_pte() or something so make it totally
> clear to the reader that they're new.
set_*_init() and set_*() I suppose.
>
> > - kernel_physical_mapping_init(__pa(vaddr & pmask),
> > - __pa((vaddr_end & pmask) + psize),
> > - split_page_size_mask);
> > + kernel_physical_mapping_change(__pa(vaddr & pmask),
> > + __pa((vaddr_end & pmask) +
> > psize),
> > + split_page_size_mask);
>
> BTW, this hunk is really nice the way that the new naming makes it more
> intuitive what's going on. My only nit w9uld be that we now have two
> very similarly-named functions with different TLB-flushing requirements.
>
> Could we please include a comment at this site that reminds us that we
> owe a TLB flush after this?
Right, a comment would be good. I think my initial proposal had the TLB
flushing inside, but I see the usage is in a loop, so I appreciate the
desire to keep the TLB flushing outside.
