Message ID | 20171018191511.20685-1-tuliom@linux.vnet.ibm.com |
---|---|
State | New |
Headers | show |
Series | powerpc: Update AT_HWCAP2 bits | expand |
On 10/18/2017 09:15 PM, Tulio Magno Quites Machado Filho wrote: > Linux commit ID XXXXXXXXX reserved a new bit for a scenario where > transactional memory is available, but the suspended state is disabled. The semantics of this bit are wrong. If the suspended state is not available, the old hwcap bit needs to be cleared, and a new flag needs to be set which documents the availability of the different for, of transactional memory. Otherwise, old applications which assume the presence of the suspended state when the old hwcap bit is set will break. (I think we should have this discussion on the kernel list.) Thanks, Florian
Florian Weimer <fweimer@redhat.com> writes: > On 10/18/2017 09:15 PM, Tulio Magno Quites Machado Filho wrote: >> Linux commit ID XXXXXXXXX reserved a new bit for a scenario where >> transactional memory is available, but the suspended state is disabled. > > The semantics of this bit are wrong. If the suspended state is not > available, the old hwcap bit needs to be cleared, and a new flag needs > to be set which documents the availability of the different for, of > transactional memory. Otherwise, old applications which assume the > presence of the suspended state when the old hwcap bit is set will break. > > (I think we should have this discussion on the kernel list.) Agreed. Especially because I don't think the points you're raising will affect the glibc patch unless it's necessary to change the name of the bit.
On 19/10/2017 07:57, Florian Weimer wrote: > On 10/18/2017 09:15 PM, Tulio Magno Quites Machado Filho wrote: >> Linux commit ID XXXXXXXXX reserved a new bit for a scenario where >> transactional memory is available, but the suspended state is disabled. > > The semantics of this bit are wrong. If the suspended state is not available, the old hwcap bit needs to be cleared, and a new flag needs to be set which documents the availability of the different for, of transactional memory. Otherwise, old applications which assume the presence of the suspended state when the old hwcap bit is set will break. > > (I think we should have this discussion on the kernel list.) > > Thanks, > Florian How exactly this kind of hardware handles the 'tsr.' instruction state change? Assuming the kernel submission proposal it seems an ABI extension, since MSR[TS] will be change from 0xb10 to 0b00 in case of a transaction (instead of expected 0b10 to 0x01). Will it also execute the failure handling in this case, as for a 'tresume' in the case a failure in suspended state? Also, will it also change TEXARS with the abort information? I completely agree with Florian here, this is as *ABI* change and the kernel need to advertise a different TM ABI instead of as an extension.
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: > On 19/10/2017 07:57, Florian Weimer wrote: >> On 10/18/2017 09:15 PM, Tulio Magno Quites Machado Filho wrote: >>> Linux commit ID XXXXXXXXX reserved a new bit for a scenario where >>> transactional memory is available, but the suspended state is disabled. >> >> The semantics of this bit are wrong. If the suspended state is not available, the old hwcap bit needs to be cleared, and a new flag needs to be set which documents the availability of the different for, of transactional memory. Otherwise, old applications which assume the presence of the suspended state when the old hwcap bit is set will break. >> >> (I think we should have this discussion on the kernel list.) I'd appreciate if we could follow Florian's suggestion and keep this discussion in linuxppc-dev mailing list, with the authors of these patches involved too. ;-) For the record, the last version of the patch series is being reviewed at [1] [2]. [1] https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-October/164701.html [2] http://patchwork.ozlabs.org/project/linuxppc-dev/list/?series=7773 > How exactly this kind of hardware handles the 'tsr.' instruction > state change? Assuming the kernel submission proposal it seems > an ABI extension, since MSR[TS] will be change from 0xb10 to 0b00 > in case of a transaction (instead of expected 0b10 to 0x01). Indeed. It traps and aborts the transaction. > Will it also execute the failure handling in this case, as for a > 'tresume' in the case a failure in suspended state? When suspend is not available, it will never enter suspended state. > Also, will it also change TEXARS with the abort information? It should, with a permanent error cause so that old applications entering suspended state can adopt another technique. > I completely agree with Florian here, this is as *ABI* change > and the kernel need to advertise a different TM ABI instead > of as an extension. I'm forwarding your last question and this comment to linuxppc-dev and Cc'ing you. Would you mind to elaborate what you're mean there, please? By the way, you may want to read my reply to Florian there [3] before posting. [3] https://lists.ozlabs.org/pipermail/linuxppc-dev/2017-October/165124.html Thank you all for these comments!
Hi Tulio, On 18-10-2017 17:15, Tulio Magno Quites Machado Filho wrote: > This patch will wait for the acceptance of the kernel patch [1] into > Linus' tree. > > [1] http://patchwork.ozlabs.org/patch/824764/ Is it really necessary to wait the TM patches into Linus' tree to push your patch as-is, given that from discussion @linuxppc-dev with Ellerman it's now clear that 'htm' feature and 'htm-no-suspend' feature are mutually exclusive? I'm just asking b/c I don't see any harm on running your patch with a kernel without TM no suspend mode support and it looks correct as-is. Regards, Gustavo > --- 8< --- > > Linux commit ID XXXXXXXXX reserved a new bit for a scenario where > transactional memory is available, but the suspended state is disabled. > > 2017-10-18 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> > > * sysdeps/powerpc/bits/hwcap.h (PPC_FEATURE2_HTM_NO_SUSPEND): New > macro. > * sysdeps/powerpc/dl-procinfo.c (_dl_powerpc_cap_flags): Add > htm-no-suspend. > > Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> > --- > sysdeps/powerpc/bits/hwcap.h | 2 ++ > sysdeps/powerpc/dl-procinfo.c | 4 ++-- > 2 files changed, 4 insertions(+), 2 deletions(-) > > diff --git a/sysdeps/powerpc/bits/hwcap.h b/sysdeps/powerpc/bits/hwcap.h > index dfc71c2..0668ca0 100644 > --- a/sysdeps/powerpc/bits/hwcap.h > +++ b/sysdeps/powerpc/bits/hwcap.h > @@ -72,3 +72,5 @@ > 128-bit */ > #define PPC_FEATURE2_DARN 0x00200000 /* darn instruction. */ > #define PPC_FEATURE2_SCV 0x00100000 /* scv syscall. */ > +#define PPC_FEATURE2_HTM_NO_SUSPEND 0x00080000 /* TM without suspended > + state. */ > diff --git a/sysdeps/powerpc/dl-procinfo.c b/sysdeps/powerpc/dl-procinfo.c > index 4dac16d..55a6e78 100644 > --- a/sysdeps/powerpc/dl-procinfo.c > +++ b/sysdeps/powerpc/dl-procinfo.c > @@ -45,7 +45,7 @@ > #if !defined PROCINFO_DECL && defined SHARED > ._dl_powerpc_cap_flags > #else > -PROCINFO_CLASS const char _dl_powerpc_cap_flags[64][10] > +PROCINFO_CLASS const char _dl_powerpc_cap_flags[64][15] > #endif > #ifndef PROCINFO_DECL > = { > @@ -61,7 +61,7 @@ PROCINFO_CLASS const char _dl_powerpc_cap_flags[64][10] > "", "", "", "", > "", "", "", "", > "", "", "", "", > - "", "", "", "", > + "", "", "", "htm-no-suspend", > "scv", "darn", "ieee128", "arch_3_00", > "htm-nosc", "vcrypto", "tar", "isel", > "ebb", "dscr", "htm", "arch_2_07", >
On 08/11/2017 14:11, Gustavo Romero wrote: > Hi Tulio, > > On 18-10-2017 17:15, Tulio Magno Quites Machado Filho wrote: >> This patch will wait for the acceptance of the kernel patch [1] into >> Linus' tree. >> >> [1] http://patchwork.ozlabs.org/patch/824764/ > > Is it really necessary to wait the TM patches into Linus' tree to push your > patch as-is, given that from discussion @linuxppc-dev with Ellerman it's > now clear that 'htm' feature and 'htm-no-suspend' feature are mutually > exclusive? > > I'm just asking b/c I don't see any harm on running your patch with a kernel > without TM no suspend mode support and it looks correct as-is. Yes, we only assume that it is de facto canonical kernel ABI if it has been accepted on Linus' tree.
Hi Zanella, On 08-11-2017 14:46, Adhemerval Zanella wrote: > > > On 08/11/2017 14:11, Gustavo Romero wrote: >> Hi Tulio, >> >> On 18-10-2017 17:15, Tulio Magno Quites Machado Filho wrote: >>> This patch will wait for the acceptance of the kernel patch [1] into >>> Linus' tree. >>> >>> [1] http://patchwork.ozlabs.org/patch/824764/ >> >> Is it really necessary to wait the TM patches into Linus' tree to push your >> patch as-is, given that from discussion @linuxppc-dev with Ellerman it's >> now clear that 'htm' feature and 'htm-no-suspend' feature are mutually >> exclusive? >> >> I'm just asking b/c I don't see any harm on running your patch with a kernel >> without TM no suspend mode support and it looks correct as-is. > > Yes, we only assume that it is de facto canonical kernel ABI if it > has been accepted on Linus' tree. I see. Thanks for the info. Regards, Gustavo
Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> writes: > This patch will wait for the acceptance of the kernel patch [1] into > Linus' tree. Kernel patch has been merged, so I'm merging the glibc patch too: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e > Linux commit ID XXXXXXXXX reserved a new bit for a scenario where > transactional memory is available, but the suspended state is disabled. > > 2017-10-18 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> > > * sysdeps/powerpc/bits/hwcap.h (PPC_FEATURE2_HTM_NO_SUSPEND): New > macro. > * sysdeps/powerpc/dl-procinfo.c (_dl_powerpc_cap_flags): Add > htm-no-suspend. > > Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com>
On 11/17/2017 12:00 PM, Tulio Magno Quites Machado Filho wrote: > Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> writes: > >> This patch will wait for the acceptance of the kernel patch [1] into >> Linus' tree. > > Kernel patch has been merged, so I'm merging the glibc patch too: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e Note that while merging to Linus tree marks the point at which glibc will accept changes per: https://sourceware.org/glibc/wiki/Style_and_Conventions#Support_for_features_not_yet_in_the_mainstream_Linux_kernel.3F ... it is not a stable ABI until you see a Linux 4.15 with that constant released. Likewise the constant in glibc master is not stable until we release this glibc on February 1st 2018. So one should be careful that this constant is *close* to being frozen forever by the glibc release and cannot be changed in the kernel, even if you wanted to.
Carlos O'Donell <carlos@redhat.com> writes: > On 11/17/2017 12:00 PM, Tulio Magno Quites Machado Filho wrote: >> Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> writes: >> >>> This patch will wait for the acceptance of the kernel patch [1] into >>> Linus' tree. >> >> Kernel patch has been merged, so I'm merging the glibc patch too: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e > > Note that while merging to Linus tree marks the point at which glibc will > accept changes per: > https://sourceware.org/glibc/wiki/Style_and_Conventions#Support_for_features_not_yet_in_the_mainstream_Linux_kernel.3F > > ... it is not a stable ABI until you see a Linux 4.15 with that constant > released. > > Likewise the constant in glibc master is not stable until we release this > glibc on February 1st 2018. > > So one should be careful that this constant is *close* to being frozen > forever by the glibc release and cannot be changed in the kernel, even > if you wanted to. Ack.
On 11/17/2017 09:00 PM, Tulio Magno Quites Machado Filho wrote: > Tulio Magno Quites Machado Filho<tuliom@linux.vnet.ibm.com> writes: > >> This patch will wait for the acceptance of the kernel patch [1] into >> Linus' tree. > Kernel patch has been merged, so I'm merging the glibc patch too: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e Could we backport this to 2.26? It's essentially a hardware errata workaround, so perhaps it qualifies as a bugfix? If not, I think we'd carry it as a customer downstream patch. Thanks, Florian
On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: > On 11/17/2017 09:00 PM, Tulio Magno Quites Machado Filho wrote: >> Tulio Magno Quites Machado Filho<tuliom@linux.vnet.ibm.com> writes: >> >>> This patch will wait for the acceptance of the kernel patch [1] into >>> Linus' tree. >> Kernel patch has been merged, so I'm merging the glibc patch too: >> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e > > Could we backport this to 2.26? It's essentially a hardware errata > workaround, so perhaps it qualifies as a bugfix? Doesn't the _dl_powerpc_cap_flags change mean that this is a (libc internal) ABI change? Andreas.
On 11/20/2017 04:53 PM, Andreas Schwab wrote: > On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: > >> On 11/17/2017 09:00 PM, Tulio Magno Quites Machado Filho wrote: >>> Tulio Magno Quites Machado Filho<tuliom@linux.vnet.ibm.com> writes: >>> >>>> This patch will wait for the acceptance of the kernel patch [1] into >>>> Linus' tree. >>> Kernel patch has been merged, so I'm merging the glibc patch too: >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e >> >> Could we backport this to 2.26? It's essentially a hardware errata >> workaround, so perhaps it qualifies as a bugfix? > > Doesn't the _dl_powerpc_cap_flags change mean that this is a (libc > internal) ABI change? Oops. I probably replied to the wrong patch then. I meant the related patch which adds the flag to an installed header. Thanks, Florian
On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: > On 11/20/2017 04:53 PM, Andreas Schwab wrote: >> On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: >> >>> On 11/17/2017 09:00 PM, Tulio Magno Quites Machado Filho wrote: >>>> Tulio Magno Quites Machado Filho<tuliom@linux.vnet.ibm.com> writes: >>>> >>>>> This patch will wait for the acceptance of the kernel patch [1] into >>>>> Linus' tree. >>>> Kernel patch has been merged, so I'm merging the glibc patch too: >>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e >>> >>> Could we backport this to 2.26? It's essentially a hardware errata >>> workaround, so perhaps it qualifies as a bugfix? >> >> Doesn't the _dl_powerpc_cap_flags change mean that this is a (libc >> internal) ABI change? > > Oops. I probably replied to the wrong patch then. I meant the related > patch which adds the flag to an installed header. It does both, so you would have to split that off. Andreas.
On 11/20/2017 06:17 PM, Andreas Schwab wrote: > On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: > >> On 11/20/2017 04:53 PM, Andreas Schwab wrote: >>> On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: >>> >>>> On 11/17/2017 09:00 PM, Tulio Magno Quites Machado Filho wrote: >>>>> Tulio Magno Quites Machado Filho<tuliom@linux.vnet.ibm.com> writes: >>>>> >>>>>> This patch will wait for the acceptance of the kernel patch [1] into >>>>>> Linus' tree. >>>>> Kernel patch has been merged, so I'm merging the glibc patch too: >>>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=5b0e2cb020085efe202123162502e0b551e49a0e >>>> >>>> Could we backport this to 2.26? It's essentially a hardware errata >>>> workaround, so perhaps it qualifies as a bugfix? >>> >>> Doesn't the _dl_powerpc_cap_flags change mean that this is a (libc >>> internal) ABI change? >> >> Oops. I probably replied to the wrong patch then. I meant the related >> patch which adds the flag to an installed header. > > It does both, so you would have to split that off. Sorry, I'm interested in the header file change. Not sure if this was clear. Thanks, Florian
On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: > Sorry, I'm interested in the header file change. Not sure if this was > clear. Then you need to split that off. Andreas.
Andreas Schwab <schwab@suse.de> writes: > On Nov 20 2017, Florian Weimer <fweimer@redhat.com> wrote: > >> Sorry, I'm interested in the header file change. Not sure if this was >> clear. > > Then you need to split that off. This patch is changing an internal structure. Could you elaborate why you think this is a problem?
On Nov 21 2017, "Tulio Magno Quites Machado Filho" <tuliom@linux.vnet.ibm.com> wrote: > This patch is changing an internal structure. > Could you elaborate why you think this is a problem? _rtld_global_ro is part of the ld.so interface. Andreas.
On 11/21/2017 12:21 PM, Andreas Schwab wrote: > On Nov 21 2017, "Tulio Magno Quites Machado Filho" <tuliom@linux.vnet.ibm.com> wrote: > >> This patch is changing an internal structure. >> Could you elaborate why you think this is a problem? > > _rtld_global_ro is part of the ld.so interface. Are you suggesting that we cannot change GLIBC_PRIVATE ABIs on a release branch? Thanks, Florian
On 21/11/17 11:25, Florian Weimer wrote: > On 11/21/2017 12:21 PM, Andreas Schwab wrote: >> On Nov 21 2017, "Tulio Magno Quites Machado Filho" <tuliom@linux.vnet.ibm.com> wrote: >> >>> This patch is changing an internal structure. >>> Could you elaborate why you think this is a problem? >> >> _rtld_global_ro is part of the ld.so interface. > > Are you suggesting that we cannot change GLIBC_PRIVATE ABIs on a release branch? anything that changes internal abi between the modules of the libc should not be observable externally in principle, but it can break things during libc update or static linked binaries that expect a particular abi from dlopened libc modules. i'd prefer if internal abi was not changed on release branches (including not breaking malloc interposition or sanitizers etc that hook into internals as much as possible).
On Nov 21 2017, Florian Weimer <fweimer@redhat.com> wrote: > On 11/21/2017 12:21 PM, Andreas Schwab wrote: >> On Nov 21 2017, "Tulio Magno Quites Machado Filho" <tuliom@linux.vnet.ibm.com> wrote: >> >>> This patch is changing an internal structure. >>> Could you elaborate why you think this is a problem? >> >> _rtld_global_ro is part of the ld.so interface. > > Are you suggesting that we cannot change GLIBC_PRIVATE ABIs on a release > branch? You will need to restart all running processes to avoid getting conflicts after such an update. Andreas.
On 11/21/2017 12:46 PM, Andreas Schwab wrote: > On Nov 21 2017, Florian Weimer <fweimer@redhat.com> wrote: > >> On 11/21/2017 12:21 PM, Andreas Schwab wrote: >>> On Nov 21 2017, "Tulio Magno Quites Machado Filho" <tuliom@linux.vnet.ibm.com> wrote: >>> >>>> This patch is changing an internal structure. >>>> Could you elaborate why you think this is a problem? >>> >>> _rtld_global_ro is part of the ld.so interface. >> >> Are you suggesting that we cannot change GLIBC_PRIVATE ABIs on a release >> branch? > > You will need to restart all running processes to avoid getting > conflicts after such an update. I think _rtld_global_ro is fairly harmless in this regard because there is only a small window during process startup, before libc.so.6 is loaded. So this will only affect processes started during the update (and perhaps dlmopen). Other GLIBC_PRIVATE changes are far more invasive because they affect anything which does a late dlopen (which can happen an arbitrary time span after the update, of course). Thanks, Florian
On Nov 21 2017, Florian Weimer <fweimer@redhat.com> wrote: > I think _rtld_global_ro is fairly harmless in this regard because there is > only a small window during process startup, before libc.so.6 is loaded. > So this will only affect processes started during the update (and perhaps > dlmopen). The problem is that the patch changes the size, and the dl-procinfo.c parts come first in _rtld_global_ro. Andreas.
diff --git a/sysdeps/powerpc/bits/hwcap.h b/sysdeps/powerpc/bits/hwcap.h index dfc71c2..0668ca0 100644 --- a/sysdeps/powerpc/bits/hwcap.h +++ b/sysdeps/powerpc/bits/hwcap.h @@ -72,3 +72,5 @@ 128-bit */ #define PPC_FEATURE2_DARN 0x00200000 /* darn instruction. */ #define PPC_FEATURE2_SCV 0x00100000 /* scv syscall. */ +#define PPC_FEATURE2_HTM_NO_SUSPEND 0x00080000 /* TM without suspended + state. */ diff --git a/sysdeps/powerpc/dl-procinfo.c b/sysdeps/powerpc/dl-procinfo.c index 4dac16d..55a6e78 100644 --- a/sysdeps/powerpc/dl-procinfo.c +++ b/sysdeps/powerpc/dl-procinfo.c @@ -45,7 +45,7 @@ #if !defined PROCINFO_DECL && defined SHARED ._dl_powerpc_cap_flags #else -PROCINFO_CLASS const char _dl_powerpc_cap_flags[64][10] +PROCINFO_CLASS const char _dl_powerpc_cap_flags[64][15] #endif #ifndef PROCINFO_DECL = { @@ -61,7 +61,7 @@ PROCINFO_CLASS const char _dl_powerpc_cap_flags[64][10] "", "", "", "", "", "", "", "", "", "", "", "", - "", "", "", "", + "", "", "", "htm-no-suspend", "scv", "darn", "ieee128", "arch_3_00", "htm-nosc", "vcrypto", "tar", "isel", "ebb", "dscr", "htm", "arch_2_07",
This patch will wait for the acceptance of the kernel patch [1] into Linus' tree. [1] http://patchwork.ozlabs.org/patch/824764/ --- 8< --- Linux commit ID XXXXXXXXX reserved a new bit for a scenario where transactional memory is available, but the suspended state is disabled. 2017-10-18 Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> * sysdeps/powerpc/bits/hwcap.h (PPC_FEATURE2_HTM_NO_SUSPEND): New macro. * sysdeps/powerpc/dl-procinfo.c (_dl_powerpc_cap_flags): Add htm-no-suspend. Signed-off-by: Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> --- sysdeps/powerpc/bits/hwcap.h | 2 ++ sysdeps/powerpc/dl-procinfo.c | 4 ++-- 2 files changed, 4 insertions(+), 2 deletions(-)