| Message ID | 20231129083055.3627888-1-mmatti@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc: Add space for HWCAP3/HWCAP4 in the TCB for future Power. | expand |
* Manjunath Matti: > This patch reserves space for HWCAP3/HWCAP4 in the TCB of powerpc. > These hardware capabilities bits will be used by future Power > architectures. > > This is an ABI change for GLIBC 2.39. Sorry, I don't see the ABI change. Peter and I discussed this a while ago, and it's possible to avoid the ABI change by indicating the presence of hwcap_extn with another flag in hwcap. Do you plan to follow this path? The patch does not make this clear. Thanks, Florian
On 11/29/23 7:55 AM, Florian Weimer wrote: >> This patch reserves space for HWCAP3/HWCAP4 in the TCB of powerpc. >> These hardware capabilities bits will be used by future Power >> architectures. >> >> This is an ABI change for GLIBC 2.39. > > Sorry, I don't see the ABI change. Hi Florian, The ABI extension is the extra space added to tcbhead_t via the new hwcap_extn field. Currently, this patch just reserves the space and initializes it to zero. Manjunath has a follow-on patch which is dependent on a linux kernel patch to define AT_HWCAP3 and AT_HWCAP4 which is still in the process of getting upstreamed. There have been no objections and the aarch64 team mentioned needing HWCAP3 soon-ish too, it's just taking a bit to get pulled. We were going to wait to post Manjunath's follow-on patch until the kernel change went in, but I guess he could submit it now so you can see what it does, but obviously, we have to wait for the kernel patch to land before pushing it to glibc, so we can reply on the AT_HWCAP3 and AT_HWCAP4 values defined by the kernel. Manjunath, can you please post your 2nd patch for review so everyone can see have a look at it now, while we wait for the kernel changes to land? Thanks. > Peter and I discussed this a while ago, and it's possible to avoid the > ABI change by indicating the presence of hwcap_extn with another flag in > hwcap. Do you plan to follow this path? The patch does not make this > clear. All true, but we decided against that solution. Currently, a call to __builtin_cpu_supports ("foo") for a HWCAP/HWCAP2 feature will expand into a load-compare-branch sequence. If we use your suggestion, we would eliminate the ABI extension, but we'd end up having to execute a load-compare-branch-load-compare-branch sequence for the HWCAP3/HWCAP4 features, which when they exist, will probably be the most commonly tested for features, essentially doubling the cost of the common case. Instead, we've decided to go with the extended ABI solution, but early enough, so that when we finally have HWCAP3 features, most (all?) of the shipping distro toolchains should already support the new HWCAP3/HWCAP4 fields in tcbhead_t. Peter
On 11/29/23 10:48 PM, Peter Bergner wrote: > On 11/29/23 7:55 AM, Florian Weimer wrote: >>> This patch reserves space for HWCAP3/HWCAP4 in the TCB of powerpc. >>> These hardware capabilities bits will be used by future Power >>> architectures. >>> >>> This is an ABI change for GLIBC 2.39. >> >> Sorry, I don't see the ABI change. > > Hi Florian, > > The ABI extension is the extra space added to tcbhead_t via the new > hwcap_extn field. Currently, this patch just reserves the space and > initializes it to zero. [snip] Hi Florian, Thinking about this more, I guess the exported symbols announcing the availability of that stack space which is currently in Manjunath's 2nd patch should probably be added to this patch, since the exported symbols are themselves also part of the ABI. Is that part of what caused this not to be clear? Peter
* Peter Bergner: > On 11/29/23 10:48 PM, Peter Bergner wrote: >> On 11/29/23 7:55 AM, Florian Weimer wrote: >>>> This patch reserves space for HWCAP3/HWCAP4 in the TCB of powerpc. >>>> These hardware capabilities bits will be used by future Power >>>> architectures. >>>> >>>> This is an ABI change for GLIBC 2.39. >>> >>> Sorry, I don't see the ABI change. >> >> Hi Florian, >> >> The ABI extension is the extra space added to tcbhead_t via the new >> hwcap_extn field. Currently, this patch just reserves the space and >> initializes it to zero. > [snip] > > Hi Florian, > > Thinking about this more, I guess the exported symbols announcing the > availability of that stack space which is currently in Manjunath's > 2nd patch should probably be added to this patch, since the exported > symbols are themselves also part of the ABI. Is that part of what > caused this not to be clear? Yes, I was confused because there was no discernible ABI change. If there's the symbol and compilers will emit references to it, then yes, this looks good to me. Thanks, Florian
On 11/30/23 11:45 AM, Florian Weimer wrote: >> On 11/29/23 10:48 PM, Peter Bergner wrote: >> Thinking about this more, I guess the exported symbols announcing the >> availability of that stack space which is currently in Manjunath's >> 2nd patch should probably be added to this patch, since the exported >> symbols are themselves also part of the ABI. Is that part of what >> caused this not to be clear? > > Yes, I was confused because there was no discernible ABI change. If > there's the symbol and compilers will emit references to it, then yes, > this looks good to me. Ok, thanks. Manjunath, can you please repost your two patches, except please move the special exported symbol from patch 2 into patch 1, since that is also part of the ABI extension. Thanks. Peter
diff --git a/sysdeps/powerpc/hwcapinfo.c b/sysdeps/powerpc/hwcapinfo.c index f2c473c556..bf46a4c84d 100644 --- a/sysdeps/powerpc/hwcapinfo.c +++ b/sysdeps/powerpc/hwcapinfo.c @@ -70,6 +70,7 @@ __tcb_parse_hwcap_and_convert_at_platform (void) /* Consolidate both HWCAP and HWCAP2 into a single doubleword so that we can read both in a single load later. */ __tcb.hwcap = (h1 << 32) | (h2 & 0xffffffff); + __tcb.hwcap_extn = 0x0; } #if IS_IN (rtld) diff --git a/sysdeps/powerpc/nptl/tcb-offsets.sym b/sysdeps/powerpc/nptl/tcb-offsets.sym index 4c01615ad0..9b29fe8d1a 100644 --- a/sysdeps/powerpc/nptl/tcb-offsets.sym +++ b/sysdeps/powerpc/nptl/tcb-offsets.sym @@ -26,3 +26,4 @@ TCB_AT_PLATFORM (offsetof (tcbhead_t, at_platform) - TLS_TCB_OFFSET - sizeof(t PADDING (offsetof (tcbhead_t, padding) - TLS_TCB_OFFSET - sizeof(tcbhead_t)) #endif TCB_HWCAP (offsetof (tcbhead_t, hwcap) - TLS_TCB_OFFSET - sizeof (tcbhead_t)) +TCB_HWCAP_EXTN (offsetof (tcbhead_t, hwcap_extn) - TLS_TCB_OFFSET - sizeof (tcbhead_t)) diff --git a/sysdeps/powerpc/nptl/tls.h b/sysdeps/powerpc/nptl/tls.h index a668798181..3f8eca57b5 100644 --- a/sysdeps/powerpc/nptl/tls.h +++ b/sysdeps/powerpc/nptl/tls.h @@ -64,6 +64,9 @@ are private. */ typedef struct { + /* Reservation for HWCAP3 and HWCAP4 data. To be accessed by GCC in + __builtin_cpu_supports(), so it is a part of the public ABI. */ + uint64_t hwcap_extn; /* Reservation for HWCAP data. To be accessed by GCC in __builtin_cpu_supports(), so it is a part of public ABI. */ uint64_t hwcap; @@ -138,6 +141,7 @@ typedef struct ({ \ __thread_register = (void *) (tcbp) + TLS_TCB_OFFSET; \ THREAD_SET_HWCAP (__tcb.hwcap); \ + THREAD_SET_HWCAP_EXTN (__tcb.hwcap_extn); \ THREAD_SET_AT_PLATFORM (__tcb.at_platform); \ true; \ }) @@ -147,6 +151,8 @@ typedef struct void *tp = (void *) (pd) + TLS_TCB_OFFSET + TLS_PRE_TCB_SIZE; \ (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].hwcap) = \ THREAD_GET_HWCAP (); \ + (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].hwcap_extn) = \ + THREAD_GET_HWCAP_EXTN (); \ (((tcbhead_t *) ((char *) tp - TLS_TCB_OFFSET))[-1].at_platform) = \ THREAD_GET_AT_PLATFORM (); @@ -189,12 +195,17 @@ typedef struct + TLS_PRE_TCB_SIZE))[-1].pointer_guard \ = THREAD_GET_POINTER_GUARD()) -/* hwcap field in TCB head. */ +/* hwcap & hwcap_extn fields in TCB head. */ # define THREAD_GET_HWCAP() \ (((tcbhead_t *) ((char *) __thread_register \ - TLS_TCB_OFFSET))[-1].hwcap) +# define THREAD_GET_HWCAP_EXTN() \ + (((tcbhead_t *) ((char *) __thread_register \ + - TLS_TCB_OFFSET))[-1].hwcap_extn) # define THREAD_SET_HWCAP(value) \ (THREAD_GET_HWCAP () = (value)) +# define THREAD_SET_HWCAP_EXTN(value) \ + (THREAD_GET_HWCAP_EXTN () = (value)) /* at_platform field in TCB head. */ # define THREAD_GET_AT_PLATFORM() \