On Mon, Jun 18, 2018 at 04:28:27PM +0000, Dave Hansen wrote:
> > index 17383f9677fa..032b9a1ba8e1 100644
> > --- a/arch/x86/mm/init_64.c
> > +++ b/arch/x86/mm/init_64.c
> > @@ -731,6 +731,8 @@ kernel_physical_mapping_init(unsigned long paddr_start,
> > pgd_changed = true;
> > }
> >
> > + sync_direct_mapping();
> > +
> > if (pgd_changed)
> > sync_global_pgds(vaddr_start, vaddr_end - 1);
> >
> > @@ -1142,10 +1144,13 @@ void __ref vmemmap_free(unsigned long start, unsigned long end,
> > static void __meminit
> > kernel_physical_mapping_remove(unsigned long start, unsigned long end)
> > {
> > + int ret;
> > start = (unsigned long)__va(start);
> > end = (unsigned long)__va(end);
> >
> > remove_pagetable(start, end, true, NULL);
> > + ret = sync_direct_mapping();
> > + WARN_ON(ret);
> > }
>
> I understand why you implemented it this way, I really do. It's
> certainly the quickest way to hack something together and make a
> standalone piece of code. But, I don't think it's maintainable.
>
> For instance, this call to sync_direct_mapping() could be entirely
> replaced by a call to:
>
> for_each_keyid(k)...
> remove_pagetable(start + offset_per_keyid * k,
> end + offset_per_keyid * k,
> true, NULL);
>
> No?
Yes. But what's the point if we need to have the sync routine anyway for
the add path?
> > int __ref arch_remove_memory(u64 start, u64 size, struct vmem_altmap *altmap)
> > @@ -1290,6 +1295,7 @@ void mark_rodata_ro(void)
> > (unsigned long) __va(__pa_symbol(rodata_end)),
> > (unsigned long) __va(__pa_symbol(_sdata)));
> >
> > + sync_direct_mapping();
> > debug_checkwx();
>
> Huh, checking the return code in some cases and not others. Curious.
> Why is it that way?
There's no sensible way to handle failure in any of these path. But in
remove path we don't expect the failure -- no allocation required.
It can only happen if we missed sync_direct_mapping() somewhere else.
--
Kirill A. Shutemov
