Re: [PATCH 2/2] x86-64/Xen: eliminate W+X mappings

[Juergen Gross]

On 12/12/17 11:48, Jan Beulich wrote:
>>>> On 12.12.17 at 11:38, <mingo@xxxxxxxxxx> wrote:
>> * Jan Beulich <JBeulich@xxxxxxxx> 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