GCS ACLE #260

nsz-arm · 2023-05-31T09:30:01Z

name: Pull request
about: Technical issues, document format problems, bugs in scripts or feature proposal.

Thank you for submitting a pull request!

If this PR is about a bugfix:

Please use the bugfix label and make sure to go through the checklist below.

If this PR is about a proposal:

We are looking forward to evaluate your proposal, and if possible to
make it part of the Arm C Language Extension (ACLE) specifications.

We would like to encourage you reading through the contribution
guidelines, in particular the section on submitting
a proposal.

Please use the proposal label.

As for any pull request, please make sure to go through the below
checklist.

Checklist: (mark with X those which apply)

If an issue reporting the bug exists, I have mentioned it in the
PR (do not bother creating the issue if all you want to do is
fixing the bug yourself).
I have added/updated the SPDX-FileCopyrightText lines on top
of any file I have edited. Format is SPDX-FileCopyrightText: Copyright {year} {entity or name} <{contact informations}>
(Please update existing copyright lines if applicable. You can
specify year ranges with hyphen , as in 2017-2019, and use
commas to separate gaps, as in 2018-2020, 2022).
I have updated the Copyright section of the sources of the
specification I have edited (this will show up in the text
rendered in the PDF and other output format supported). The
format is the same described in the previous item.
I have run the CI scripts (if applicable, as they might be
tricky to set up on non-*nix machines). The sequence can be
found in the contribution
guidelines. Don't
worry if you cannot run these scripts on your machine, your
patch will be automatically checked in the Actions of the pull
request.
I have added an item that describes the changes I have
introduced in this PR in the section Changes for next
release of the section Change Control/Document history
of the document. Create Changes for next release if it does
not exist. Notice that changes that are not modifying the
content and rendering of the specifications (both HTML and PDF)
do not need to be listed.
When modifying content and/or its rendering, I have checked the
correctness of the result in the PDF output (please refer to the
instructions on how to build the PDFs
locally).
The variable draftversion is set to true in the YAML header
of the sources of the specifications I have modified.
Please DO NOT add my GitHub profile to the list of contributors
in the README page of the project.

DanielKristofKiss

LGTM with small comments.

DanielKristofKiss · 2023-05-31T09:52:02Z

main/acle.md

+
+To use the intrinsics, `arm_acle.h` needs to be included.
+
+The intrinsics are available if `__ARM_FEATURE_GCS_DEFAULT` is defined,


Intrinsics would be always available because there could be cases when the intrinsics is called in the right context but the whole translation unit is not compiled with GCS. (e.g. target attribute or FMV)

i'm not against making intrinsics always available, but for GCS the intrinsics are only useful if GCS is enabled, and that can only be enabled if everything is compiled in GCS compatible mode, so there is not really any context when __ARM_FEATURE_GCS_DEFAULT is undef but you want the intrinsics anyway.

DanielKristofKiss · 2023-05-31T09:56:46Z

main/acle.md

+but they can only be called if GCS instructions are supported.
+
+The `__chkfeat` intrinsics with `_CHKFEAT_GCS` can be used to check
+if GCS protection is enabled at runtime.  If GCS protection is enabled


superfluous space before If GCS

DanielKristofKiss · 2023-05-31T09:56:48Z

main/acle.md

+```
+
+Reads and returns the last entry on the GCS of the current thread and
+updates the GCS pointer to point to the previous entry.  If GCS


superfluous space before If GCS

DanielKristofKiss · 2023-05-31T09:56:50Z

main/acle.md

+```
+
+Switches the GCS of the current thread, where the argument is the new
+GCS pointer, and returns the old GCS pointer.  If GCS protection is


superfluous space before If GCS

john-brawn-arm · 2023-10-19T10:49:08Z

main/acle.md

@@ -2937,6 +2948,18 @@ inclusive. See implementation documentation for the effect (if any) of
 this instruction and the meaning of the argument. This is available only
 when compiling for AArch32.

+``` c
+  uint64_t __chkfeat(uint64_t);


The behaviour of this intrinsic is opposite that of the instruction (chkfeat instruction clears the bit if the feature is enabled), which I think is unique among intrinsics. It would be worthwhile specifically noting this.

Also "feature is available" should be "feature is enabled" (as the chkfeat instruction will leave the bit alone if the feature is available but disabled).

I'd prefer to write/read this code in applications:
if( __chkfeat(_CHKFEAT_GCS)) { // do GCS stuff. }
chkfeat must clear the bit if the feature is present due to it is a NOP on old CPUs where it won't change the register content.
That logic twist is not required for the app developers.

+1 for the

Also "feature is available" should be "feature is enabled"

john-brawn-arm · 2023-10-19T11:11:40Z

main/acle.md

+
+Reads and returns the last entry on the GCS of the current thread and
+updates the GCS pointer to point to the previous entry. If GCS
+protection is disabled then it has no side effect and returns `0`.


The gcspopm instruction does nothing when gcs is disabled (i.e. the value in the destination register is unchanged). Is the intent that the intrinsic sets the destination register to zero before executing the instruction?

yes, i think this is the only way the intrinsic is usable in the "asynchronously disabled gcs" scenario.

(which we don't know yet if linux will support, currently it does not want to)

nsz-arm · 2023-10-19T14:26:13Z

main/acle.md

+
+
+``` c
+  const void *__gcspr(void);


i plan to change this back to non-const so that the intrinsics support the gcs write access enabled case (even if that's not expected to be a common setting).

DanielKristofKiss

LGTM, let see what others think.
NIT maybe the draft could be dropped now from the name of the PR.

nsz-arm · 2023-11-15T16:43:10Z

NIT maybe the draft could be dropped now from the name of the PR.

potential issues with current spec:

gcs store intrinsic is missing
const void* vs void* gcs (modified by bl/ret/... instructions, but normal c language stores don't work on it)
intrinsic availability check (feature test macros no longer imply intrinsics)

john-brawn-arm · 2024-06-04T15:36:55Z

gcs store intrinsic is missing

I'm not sure we need this. We can always add it later if there's a need.

const void* vs void* gcs (modified by bl/ret/... instructions, but normal c language stores don't work on it)

I think we only really need the pointer to be non-const if we have a gcs store intrinsic. constness is only about if the user is permitted to write to something, so entries on the gcs stack being modified by bl/ret isn't relevant.

intrinsic availability check (feature test macros no longer imply intrinsics)

Is this about #214? I think the way GCS intrinsics are currently described is OK with regards to that (though I have a couple of comments on the wording).

john-brawn-arm · 2024-06-04T14:41:34Z

main/acle.md

+
+To use the intrinsics, `arm_acle.h` needs to be included.
+
+The intrinsics are only valid to call when GCS instructions are


"The intrinsics are only valid to call" should be "These intrinsics are available" (that's the wording we use elsewhere).

john-brawn-arm · 2024-06-04T15:08:20Z

main/acle.md

+to check if GCS protection is enabled at runtime. If GCS protection is
+enabled then GCS instructions are supported and the code was compiled
+in a GCS protection compatible way (`__ARM_FEATURE_GCS_DEFAULT` was
+defined).


The final sentence here is a bit weird and I'm not sure what exactly it's saying. I think what it's saying is that "GCS enabled implies GCS available", i.e. if you have if (__chkfeat(_CHKFEAT_GCS)) { } then you know GCS must be available inside that block, or perhaps the other way around that if you know GCS isn't available then you know __chkfeat(_CHKFEAT_GCS) will never return true.

yeah this should be worded better, or removed.

i was trying to point out that the runtime will only enable GCS if all code is built with GCS support, so if __chkfeat check returns true, that implies __ARM_FEATURE_GCS_DEFAULT, no need to check that separately.

john-brawn-arm · 2024-06-20T15:16:50Z

LGTM.

DanielKristofKiss

LGTM

DanielKristofKiss · 2024-06-20T19:01:01Z

main/acle.md

+
+`__ARM_FEATURE_GCS_DEFAULT` is defined to `1` if the code generation is
+compatible with enabling the Guarded Control Stack (GCS) extension based
+protection.  It is undefined otherwise.


Suggested change

protection. It is undefined otherwise.

protection. It is undefined otherwise.

DanielKristofKiss · 2024-06-20T19:01:15Z

main/acle.md

+protection.  It is undefined otherwise.
+
+`__ARM_FEATURE_GCS` is defined to `1` if the Guarded Control Stack (GCS)
+extension is available on the target.  It is undefined otherwise.


Suggested change

extension is available on the target. It is undefined otherwise.

extension is available on the target. It is undefined otherwise.

This adds intrinsics defined in ARM-software/acle#260 Doing this requires some changes to the GCS instruction definitions, as these intrinsics make use of how some instructions don't modify the input register when GCS is disabled, and they need to be correctly marked with mayLoad/mayStore/hasSideEffects for instruction selection to work.

DanielKristofKiss reviewed May 31, 2023

View reviewed changes

vhscampos added the proposal label Jun 7, 2023

nsz-arm force-pushed the gcs branch from c14d570 to bf51bc3 Compare June 27, 2023 08:31

john-brawn-arm reviewed Oct 19, 2023

View reviewed changes

nsz-arm commented Oct 19, 2023

View reviewed changes

nsz-arm force-pushed the gcs branch from bf51bc3 to ab198c5 Compare November 14, 2023 14:30

DanielKristofKiss approved these changes Nov 14, 2023

View reviewed changes

john-brawn-arm reviewed Jun 4, 2024

View reviewed changes

[main] Add __chkfeat hint intrinsic

e215858

nsz-arm force-pushed the gcs branch from ab198c5 to 506ef18 Compare June 12, 2024 12:46

nsz-arm changed the title ~~GCS ACLE draft~~ GCS ACLE Jun 12, 2024

DanielKristofKiss approved these changes Jun 20, 2024

View reviewed changes

nsz-arm added 3 commits June 21, 2024 10:46

[main] Add Guarded Control Stack feature test macros

04101a9

[main] Add Guarded Control Stack intrinsics

978182a

Add GCS design document

2f1b631

nsz-arm force-pushed the gcs branch from 506ef18 to 2f1b631 Compare June 21, 2024 09:47

john-brawn-arm mentioned this pull request Jun 27, 2024

[AArch64] Implement GCS ACLE intrinsics llvm/llvm-project#96903

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCS ACLE #260

GCS ACLE #260

nsz-arm commented May 31, 2023

DanielKristofKiss left a comment

DanielKristofKiss May 31, 2023

nsz-arm May 31, 2023

DanielKristofKiss May 31, 2023

DanielKristofKiss May 31, 2023

DanielKristofKiss May 31, 2023

john-brawn-arm Oct 19, 2023

DanielKristofKiss Oct 19, 2023

john-brawn-arm Oct 19, 2023

nsz-arm Oct 19, 2023

nsz-arm Oct 19, 2023

DanielKristofKiss left a comment

nsz-arm commented Nov 15, 2023

john-brawn-arm commented Jun 4, 2024 •

edited

Loading

john-brawn-arm Jun 4, 2024

john-brawn-arm Jun 4, 2024

nsz-arm Jun 10, 2024

john-brawn-arm commented Jun 20, 2024

DanielKristofKiss left a comment

DanielKristofKiss Jun 20, 2024

DanielKristofKiss Jun 20, 2024


		To use the intrinsics, `arm_acle.h` needs to be included.

		The intrinsics are available if `__ARM_FEATURE_GCS_DEFAULT` is defined,


		To use the intrinsics, `arm_acle.h` needs to be included.

		The intrinsics are only valid to call when GCS instructions are

	protection. It is undefined otherwise.
	protection. It is undefined otherwise.

	extension is available on the target. It is undefined otherwise.
	extension is available on the target. It is undefined otherwise.

GCS ACLE #260

Are you sure you want to change the base?

GCS ACLE #260

Conversation

nsz-arm commented May 31, 2023

DanielKristofKiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DanielKristofKiss left a comment

Choose a reason for hiding this comment

nsz-arm commented Nov 15, 2023

john-brawn-arm commented Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-brawn-arm commented Jun 20, 2024

DanielKristofKiss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

john-brawn-arm commented Jun 4, 2024 •

edited

Loading