[PATCH v3] aarch64: Add support for memory protection keys

[Adhemerval Zanella Netto]

On 07/10/24 13:18, Yury Khrustalev wrote:
> Thank you for your feedback, please see my comments below.
> 
> On Tue, Oct 01, 2024 at 04:45:57PM -0300, Adhemerval Zanella Netto wrote:
>>
>>
>> On 26/09/24 09:13, yury.khrustalev@arm.com wrote:
>>> From: Yury Khrustalev <yury.khrustalev@arm.com>
>>>
>>> This patch adds support for memory protection keys on AArch64 targets
>>> where it can be implemented by hardware with enabled Permission Overlay
>>> Extension introduced in Armv8.9 / Armv9.4 [1].
>>
>> I think it would be worth to add the kernel commit and Linux version that
>> it is supported or expected to be support.  It seems that it was enabled
>> by bf83dae90fbc01d66477a3440eaad07da6657fdc on Linus tree, which is 
>> scheduled for 6.12.  So I think we will need 6.12 release before installing
>> this patch.
> 
> I've mentioned that the corresponding Linux patches are in linux-next in the
> next/20240924 branch. Support for memory pkeys has been added in a series of
> patches that include the commit that you quoted and the latest commit of them
> is b9b9d72de32bcb63ed4d9761907a3e5f352c6f9a. I'll provide commit information
> in the next version of this patch.

Thanks, usually for newer kernel features we expect them to be on official
Linus tree before get them on glibc.

> 
>>
>>>
>>>  1. Internal functions "pkey_read" and "pkey_write" to access data
>>>     associated with memory protection keys.
>>>  2. Implementation of API functions "pkey_get" and "pkey_set" for
>>>     the AArch64 target.
>>>  3. AArch64-specific PKEY flags for READ and EXECUTE (see below).
>>>  4. New target-specific test that checks behaviour of pkeys on
>>>     AArch64 targets.
>>>  5. This patch also extends existing generic test for pkeys.
>>>  6. HWCAP constant for Permission Overlay Extension feature.
>>>
>>> To support more accurate mapping of underlying permissions to the
>>> PKEY flags, we introduce AArch64-specific flags:
>>>
>>>  - PKEY_UNRESTRICTED: 0x0 (for completeness)
>>>  - PKEY_DISABLE_ACCESS: 0x1 (existing flag)
>>>  - PKEY_DISABLE_WRITE: 0x2 (existing flag)
>>>  - PKEY_DISABLE_EXECUTE: 0x4 (new flag)
>>>  - PKEY_DISABLE_READ: 0x8 (new flag)
>>
>> These new flags need to be documented on pkey_set section at
>> manual/memory.texi, as an aarch64 extension.  The pkey already have 
>> some arch-specific semantic that make portability not straightforward 
>> (like the signal handling), so I am wondering if we should just make
>> the new flags not aarch64 specific. Both x86 and powerpc already returns 
>> EINVAL for larger rights.
> 
> I'll add documentation for the flags in the next version of the patch. I'm
> not sure how to go about making them not arch specific though. This would
> be the most desirable outcome but, since the added flags are not supported
> on some targets, would it not confuse users of this interface?

It is just currently the two supported architectures (x86 and powerpc)
support the same PKEY_* flags, so they is no need for extra handling
by the caller to underlying supports.

But I don't have a strong opinion, maybe it is indeed better to make
the PKEY_* arch-specific.

> 
>>> Reverse mapping tries to retain backward compatibility and ORs
>>> PKEY_DISABLE_ACCESS whenever both flags PKEY_DISABLE_READ and
>>> PKEY_DISABLE_WRITE would be present.
>>>
>>> This will break code that compares pkey_get output with == instead
>>> of using bitwise operations. The latter is more correct since PKEY
>>> constants are documented as flags.
>>
>> Right, this another possible pitfall that it would be good to have
>> it documented on manual.
> 
> I'll document this change as well in the new version of the patch.
> 
>> I am still configuring an enviroment where I can actually tests it, but
>> besides the points above and some style issues below patch looks ok.
> 
> Thank you. I'll fix the style issues in the upcoming new version of this
> patch.
> 
> 
> Kind regards,
> Yury
>