Re: [Qemu-arm] [PATCH 01/18] armv7m: MRS/MSR handle unprivileged access

[Peter Maydell]

On 2 December 2015 at 22:51, Michael Davidsaver <address@hidden> wrote:
>
>
> On 11/17/2015 12:09 PM, Peter Maydell wrote:
>> On 9 November 2015 at 01:11, Michael Davidsaver <address@hidden> wrote:
>>> The MRS and MSR instruction handling isn't checking
>>> the current permission level.
>>>
>>> Signed-off-by: Michael Davidsaver <address@hidden>
>>> ---
>>>  target-arm/helper.c | 79 
>>> +++++++++++++++++++++++++----------------------------
>>>  1 file changed, 37 insertions(+), 42 deletions(-)
>>
>> This patch looks good overall, but there's one style nit:
>>
>>> +    case 0 ... 7: /* xPSR sub-fields */
>>> +        mask = 0;
>>> +        if ((reg&1) && el) {
>>
>> you want spaces around operators, so "reg & 1" here and elsewhere.
>
> Would be nice if checkpatch.pl caught these, but I understand that
> this would be quite difficult to do well.  I've tried to catch this
> with grep and sort through the false positives.  I think I got them
> all.

It looks like this is a case where checkpatch.pl is applying
Linux kernel style (since that's where we borrowed it from)
which is looser than ours, or at least looser than what I think
our style is. There's a specific check:

       # << and >> may either have or not have spaces both sides
    } elsif ($op eq '<<' or $op eq '>>' or
             $op eq '&' or $op eq '^' or $op eq '|' or
             $op eq '+' or $op eq '-' or
             $op eq '*' or $op eq '/' or
             $op eq '%')

and those operators are only checked for "must have consistent
spacing both sides" rather than "must have spaces on both sides".

thanks
-- PMM