Messages in this thread | | | Subject | Re: [PATCH 2/2] x86-64/Xen: eliminate W+X mappings | From | Juergen Gross <> | Date | Tue, 12 Dec 2017 14:14:54 +0100 |
| |
On 12/12/17 11:48, Jan Beulich wrote: >>>> On 12.12.17 at 11:38, <mingo@kernel.org> wrote: >> * Jan Beulich <JBeulich@suse.com> wrote: >>> --- 4.15-rc3/arch/x86/xen/mmu_pv.c >>> +++ 4.15-rc3-x86_64-Xen-avoid-W+X/arch/x86/xen/mmu_pv.c >>> @@ -1902,6 +1902,16 @@ void __init xen_setup_kernel_pagetable(p >>> /* Graft it onto L4[511][510] */ >>> copy_page(level2_kernel_pgt, l2); >>> >>> + /* Zap execute permission from the ident map. Due to the sharing of >>> + * L1 entries we need to do this in the L2. */ >> >> please use the customary (multi-line) comment style: >> >> /* >> * Comment ..... >> * ...... goes here. >> */ >> >> specified in Documentation/CodingStyle. > > I would have but didn't because all other comments in this function > use this (wrong) style. I've concluded that consistency is better > here than matching the style doc. If the Xen maintainers tell me > otherwise, I'll happily adjust the patch.
Yes, please use the correct style with new comments.
> >>> + if (__supported_pte_mask & _PAGE_NX) >>> + for (i = 0; i < PTRS_PER_PMD; ++i) { >>> + if (pmd_none(level2_ident_pgt[i])) >>> + continue; >>> + level2_ident_pgt[i] = >>> + pmd_set_flags(level2_ident_pgt[i], _PAGE_NX); >> >> So the line break here is quite distracting, especially considering how similar it >> is to the alignment of the 'continue' statement. I.e. visually it looks like >> control flow alignment. >> >> Would be much better to just leave it a single page and ignore checkpatch >> here. > > Again I'll wait to see what the Xen maintainers think. I too dislike > line splits like this one, but the line ended up quite a bit too long, > not just a character or two. I also wasn't sure whether splitting > between the function arguments would be okay, leaving the first > line just slightly too long.
That would result in a 80 character line, which IMHO is the best choice here.
Juergen
| |