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

[Juergen Gross <jgross@xxxxxxxx>]

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

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel