Messages in this thread | | | Subject | Re: [PATCH 10/34] x86, pkeys: arch-specific protection bitsy | From | Dave Hansen <> | Date | Tue, 8 Dec 2015 08:34:41 -0800 |
| |
On 12/08/2015 07:15 AM, Thomas Gleixner wrote: > On Thu, 3 Dec 2015, Dave Hansen wrote: >> >> +static inline int vma_pkey(struct vm_area_struct *vma) > > Shouldn't this return something unsigned?
Ingo had asked that we use 'int' in the syscalls at some point. We also use a -1 to mean "no pkey set" (to differentiate it from pkey=0) at least at the very top of the syscall level.
>> +{ >> + u16 pkey = 0; >> +#ifdef CONFIG_X86_INTEL_MEMORY_PROTECTION_KEYS >> + unsigned long vma_pkey_mask = VM_PKEY_BIT0 | VM_PKEY_BIT1 | >> + VM_PKEY_BIT2 | VM_PKEY_BIT3; >> + /* >> + * ffs is one-based, not zero-based, so bias back down by 1. >> + */ >> + int vm_pkey_shift = __builtin_ffsl(vma_pkey_mask) - 1; > > Took me some time to figure out that this will resolve to a compile > time constant (hopefully). Is there a reason why we don't have a > VM_PKEY_SHIFT constant in the header file which makes that code just > simple and intuitive?
All of the VM_* flags are #defined as bitmaps directly and don't define shifts:
#define VM_MAYWRITE 0x00000020 #define VM_MAYEXEC 0x00000040 #define VM_MAYSHARE 0x00000080 ...
So to get a shift we've either got to do a ffs somewhere, or we have to define the VM_PKEY_BIT*'s differently from all of the other VM_* flags. Or, we do something along the lines of:
#define VM_PKEY_BIT0 0x100000000UL #define __VM_PKEY_SHIFT (32)
and we run a small risk that somebody will desynchronize the shift and the bit definition.
We only need this shift in this *one* place, so that's why I opted for the local variable and ffs.
>> + /* >> + * gcc generates better code if we do this rather than: >> + * pkey = (flags & mask) >> shift >> + */ >> + pkey = (vma->vm_flags >> vm_pkey_shift) & >> + (vma_pkey_mask >> vm_pkey_shift); > > My gcc (4.9) does it the other way round for whatever reason.
I'll go recheck.
| |