Add support for aarch64c ASM dispatch. #1
Conversation
|
This change appears to be at the root of by Xorg crash. I haven't bottomed out whether these changes directly or indirectly are causing the issue. I needed to use an additional register in the stub, that may need to be saved and restored. |
gcjenkinson
force-pushed
the
v1.6.0-cheriabi-gentry-asm
branch
5 times, most recently
from
eeb9245
to
69db081
Compare
|
@gcjenkinson Is this still a draft? I've noticed changes but I'm not sure if it's ready for review. |
|
Let me check. I have a good version of these changes, though I suspect I removed some things as the patching is a little tricky and I left that for another time. |
gcjenkinson
force-pushed
the
v1.6.0-cheriabi-gentry-asm
branch
2 times, most recently
from
fa91a79
to
237ce3f
Compare
The existing aarch64 ASM dispatch mechanism has been adapted for the Morello ISA, replacing loads and stores with instructions to load and store capability values. In a number of cases values are computed as offsets from a sentry value, which results in an invalid capability. This code has been changed to recomputed capabilities using the program counter, before (where necessary) resealing the capability to an sentry. Save and restore the full set of caller-saved registers, including c9.
gcjenkinson
force-pushed
the
v1.6.0-cheriabi-gentry-asm
branch
from
237ce3f
to
371dead
Compare
src/GLX/glvnd_genentry.c
Outdated
| "adrp c16, entrypointFunctions + " slot "*16\n" \ | ||
| "str c17, [csp, #-16]\n" \ | ||
| "adrp c17, 0\n" \ | ||
| "scvalue c17, c17, x16\n" \ |
There was a problem hiding this comment.
I don't understand this. c16 is the result of an adrp, so why do we need to do another adrp and scvalue it? The metadata will be identical.
| const ptraddr_t stub_addr = sentry_addr + (index * STUB_SIZE); | ||
| const void* pcc = __builtin_cheri_program_counter_get(); | ||
| uintptr_t result_cap = (uintptr_t) __builtin_cheri_address_set(pcc, stub_addr); | ||
| return (GLVNDentrypointStub) __builtin_cheri_seal_entry(result_cap | 1); |
There was a problem hiding this comment.
Why do we need | 1? If glx_entrypoint_start is a sentry then it should already have the right LSB.
There was a problem hiding this comment.
Or is it because glx_entrypoint_start is a data symbol?
There was a problem hiding this comment.
Regardless, the | 1 also is only for Morello, not all CHERI...
| "adrp c0, :got:_glapi_Current\n\t" \ | ||
| "ldr c0, [c0, #:got_lo12:_glapi_Current]\n\t" \ | ||
| "ldr c0, [c0]\n\t" \ | ||
| "gcvalue x1, c0\n\t" \ |
| #if defined(__CHERI_PURE_CAPABILITY__) | ||
| const ptraddr_t sentry_addr = __builtin_cheri_address_get(glx_entrypoint_start); | ||
| const ptraddr_t stub_addr = sentry_addr + (index * STUB_SIZE); | ||
| const void* pcc = __builtin_cheri_program_counter_get(); |
There was a problem hiding this comment.
void *pcc
And do these really all need to be const? Sure, it's correct to, but the code style doesn't seem to do that from the context I can see. Especially for pcc, you've made it const void * not void * const (nor const void * const).
Changes addressing review comments: - Perform gcvalue with x0 rather than x1. - Remove unneccessary adrp with c17.
gcjenkinson
force-pushed
the
v1.6.0-cheriabi-gentry-asm
branch
from
5341995
to
88701e8
Compare
| "adrp c0, :got:_glapi_Current\n\t" \ | ||
| "ldr c0, [c0, #:got_lo12:_glapi_Current]\n\t" \ | ||
| "ldr c0, [c0]\n\t" \ | ||
| "gcvalue x0, c0\n\t" \ |
There was a problem hiding this comment.
That's not what I meant, clearly that doesn't work because 11f expects c0 to still be a valid capability. What I mean is that instead of doing gcvalue xM, cN you can delete the instruction and substitute xM with xN (provided cN remains the same capability, which it obviously is here).
The existing aarch64 ASM dispatch mechanism has been adapted for the Morello ISA, replacing loads and stores with instructions to load and store capability values.
In a number of cases values are computed as offsets from a sentry value, which results in an invalid capability. This code has been changed to recomputed capabilities using the program counter, before (where necessary) resealing the capability to an sentry.