Messages in this thread | | | Date | Sat, 5 Jan 2013 14:21:52 +0100 | From | Borislav Petkov <> | Subject | Re: [PATCH v7u1 04/31] x86, 64bit, mm: add generic kernel/ident mapping helper |
| |
On Fri, Jan 04, 2013 at 02:19:17PM -0800, Yinghai Lu wrote: > On Fri, Jan 4, 2013 at 1:19 PM, Borislav Petkov <bp@alien8.de> wrote: > > On Thu, Jan 03, 2013 at 04:48:24PM -0800, Yinghai Lu wrote: > >> +int kernel_mapping_init(pgd_t *pgd_page, unsigned long addr, unsigned long end) > >> +{ > >> + struct x86_mapping_info info = { > >> + .alloc_pgt_page = alloc_pgt_page, > >> + .pmd_flag = __PAGE_KERNEL_LARGE, > >> + .kernel_mapping = true, > >> + }; > >> + > >> + return kernel_ident_mapping_init(&info, pgd_page, addr, end); > > > > This patch looks good so far except this: > > kernel_ident_mapping_init says it initializes ident mapping but > > this is wrong and the type of mapping is actually controlled by > > info.kernel_mapping. > > it is not wrong, and it could do two things. > kernel_mapping > ident_mapping
I know that. But it has kernel_ident_mapping in the name although it can do both. IMO, it should be like this:
int kernel_mapping_init_range(pgd_t *pgd_page, unsigned long addr, unsigned long end)
... return kernel_mapping_init(&info, pgd_page, addr, end);
so that your workhorse is kernel_mapping_init and it can do both kernel and ident mapping and the wrapper is kernel_mapping_init_range() which gets a range of (addr, end), preps the &info descriptor and calls the workhorse.
See what I mean?
> > So this function which gets &info, etc should be called > > kernel_mapping_init, AFAICT. And wrt the one wrapping > > kernel_ident_mapping_init, I can't seem to find where it is called. > > What's up? > > this kernel_mapping_init is for -v8 ..., should be dropped if -v7 is > used at last.
Well, this *DEFINITELY* should be in the commit message so that reviewers don't go crazy looking for functions used in other branches. Maan, this was absolutely insane.
-- Regards/Gruss, Boris.
| |