From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755049AbdLLNPE (ORCPT ); Tue, 12 Dec 2017 08:15:04 -0500 Received: from mx2.suse.de ([195.135.220.15]:44821 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754935AbdLLNO6 (ORCPT ); Tue, 12 Dec 2017 08:14:58 -0500 Subject: Re: [PATCH 2/2] x86-64/Xen: eliminate W+X mappings To: Jan Beulich , Ingo Molnar Cc: mingo@elte.hu, tglx@linutronix.de, xen-devel , Boris Ostrovsky , linux-kernel@vger.kernel.org, hpa@zytor.com References: <5A2FBC570200007800196B3E@prv-mh.provo.novell.com> <5A2FBE540200007800196B52@prv-mh.provo.novell.com> <20171212103819.an2xxafjv3cdkuy7@gmail.com> <5A2FC2280200007800196BB8@suse.com> From: Juergen Gross Message-ID: Date: Tue, 12 Dec 2017 14:14:54 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.4.0 MIME-Version: 1.0 In-Reply-To: <5A2FC2280200007800196BB8@suse.com> Content-Type: text/plain; charset=utf-8 Content-Language: de-DE Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 12/12/17 11:48, Jan Beulich wrote: >>>> On 12.12.17 at 11:38, wrote: >> * Jan Beulich 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