| Message ID | 20191211184027.20130-13-catalin.marinas@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Memory Tagging Extension user-space support | expand |
On Wed, Dec 11, 2019 at 7:40 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > From: Vincenzo Frascino <vincenzo.frascino@arm.com> > > Add MTE-specific SIGSEGV codes to siginfo.h. > > Note that the for MTE we are reusing the same SPARC ADI codes because > the two functionalities are similar and they cannot coexist on the same > system. > > Cc: Arnd Bergmann <arnd@arndb.de> > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > [catalin.marinas@arm.com: renamed precise/imprecise to sync/async] > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > --- > include/uapi/asm-generic/siginfo.h | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) > > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > index cb3d6c267181..a5184a5438c6 100644 > --- a/include/uapi/asm-generic/siginfo.h > +++ b/include/uapi/asm-generic/siginfo.h > @@ -227,8 +227,13 @@ typedef struct siginfo { > # define SEGV_PKUERR 4 /* failed protection key checks */ > #endif > #define SEGV_ACCADI 5 /* ADI not enabled for mapped object */ > -#define SEGV_ADIDERR 6 /* Disrupting MCD error */ > -#define SEGV_ADIPERR 7 /* Precise MCD exception */ > +#ifdef __aarch64__ > +# define SEGV_MTEAERR 6 /* Asynchronous MTE error */ > +# define SEGV_MTESERR 7 /* Synchronous MTE exception */ > +#else > +# define SEGV_ADIDERR 6 /* Disrupting MCD error */ > +# define SEGV_ADIPERR 7 /* Precise MCD exception */ > +#endif SEGV_ADIPERR/SEGV_ADIDERR were added together with SEGV_ACCADI, it seems a bit odd to make only two of them conditional but not the others. I think we are generally working towards having the same constants across architectures even for features that only exist on one of them. Adding Al and Eric to Cc, maybe they have another suggestion on what constants should be used. Arnd
On Wed, Dec 11, 2019 at 08:31:28PM +0100, Arnd Bergmann wrote: > On Wed, Dec 11, 2019 at 7:40 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > From: Vincenzo Frascino <vincenzo.frascino@arm.com> > > > > Add MTE-specific SIGSEGV codes to siginfo.h. > > > > Note that the for MTE we are reusing the same SPARC ADI codes because > > the two functionalities are similar and they cannot coexist on the same > > system. > > > > Cc: Arnd Bergmann <arnd@arndb.de> > > Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> > > [catalin.marinas@arm.com: renamed precise/imprecise to sync/async] > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > --- > > include/uapi/asm-generic/siginfo.h | 9 +++++++-- > > 1 file changed, 7 insertions(+), 2 deletions(-) > > > > diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h > > index cb3d6c267181..a5184a5438c6 100644 > > --- a/include/uapi/asm-generic/siginfo.h > > +++ b/include/uapi/asm-generic/siginfo.h > > @@ -227,8 +227,13 @@ typedef struct siginfo { > > # define SEGV_PKUERR 4 /* failed protection key checks */ > > #endif > > #define SEGV_ACCADI 5 /* ADI not enabled for mapped object */ > > -#define SEGV_ADIDERR 6 /* Disrupting MCD error */ > > -#define SEGV_ADIPERR 7 /* Precise MCD exception */ > > +#ifdef __aarch64__ > > +# define SEGV_MTEAERR 6 /* Asynchronous MTE error */ > > +# define SEGV_MTESERR 7 /* Synchronous MTE exception */ > > +#else > > +# define SEGV_ADIDERR 6 /* Disrupting MCD error */ > > +# define SEGV_ADIPERR 7 /* Precise MCD exception */ > > +#endif > > SEGV_ADIPERR/SEGV_ADIDERR were added together with SEGV_ACCADI, > it seems a bit odd to make only two of them conditional but not the others. Ah, I missed this. I think we should drop the #ifdef entirely. There is no harm in having two different macros with the same value. > I think we are generally working towards having the same constants > across architectures even for features that only exist on one of them. I'd rather keep both the ARM and SPARC naming here as the behaviour may be subtly different between the two architectures. IIUC, the disrupting SPARC MCD error on means a memory corruption trap sent to the hypervisor. On ARM MTE, the asynchronous tag check fault is a pretty much benign setting of a status flag. The kernel, when detecting this flag, injects a SIGSEGV on the ret_to_user path. If there's no switch into the kernel, a user program cannot become aware of the asynchronous MTE tag check fault. We also don't have the equivalent of ACCADI.
Arnd Bergmann <arnd@arndb.de> writes: > On Wed, Dec 11, 2019 at 7:40 PM Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> From: Vincenzo Frascino <vincenzo.frascino@arm.com> >> >> Add MTE-specific SIGSEGV codes to siginfo.h. >> >> Note that the for MTE we are reusing the same SPARC ADI codes because >> the two functionalities are similar and they cannot coexist on the same >> system. Please Please Please don't do that. It is actively harmful to have architecture specific si_code values. As it makes maintenance much more difficult. Especially as the si_codes are part of union descrimanator. If your functionality is identical reuse the numbers otherwise please just select the next numbers not yet used. We have at least 256 si_codes per signal 2**32 if we really need them so there is no need to be reuse numbers. The practical problem is that architecture specific si_codes start turning kernel/signal.c into #ifdef soup, and we loose a lot of basic compile coverage because of that. In turn not compiling the code leads to bit-rot in all kinds of weird places. Now as far as the observation that this is almost the same as other functionality why can't this fit the existing interface exposed to userspace? Sometimes there are good reasons, but technology gets a lot more uptake and testing when the same interfaces are more widely available. Eric p.s. As for coexistence there is always the possibility that one chip in a cpu family does supports one thing and another chip in a cpu family supports another. So userspace may have to cope with the situation even if an individual chip doesn't. I remember a similar case where sparc had several distinct page table formats and we had a single kernel that had to cope with them all. >> Cc: Arnd Bergmann <arnd@arndb.de> >> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com> >> [catalin.marinas@arm.com: renamed precise/imprecise to sync/async] >> Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> >> --- >> include/uapi/asm-generic/siginfo.h | 9 +++++++-- >> 1 file changed, 7 insertions(+), 2 deletions(-) >> >> diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h >> index cb3d6c267181..a5184a5438c6 100644 >> --- a/include/uapi/asm-generic/siginfo.h >> +++ b/include/uapi/asm-generic/siginfo.h >> @@ -227,8 +227,13 @@ typedef struct siginfo { >> # define SEGV_PKUERR 4 /* failed protection key checks */ >> #endif >> #define SEGV_ACCADI 5 /* ADI not enabled for mapped object */ >> -#define SEGV_ADIDERR 6 /* Disrupting MCD error */ >> -#define SEGV_ADIPERR 7 /* Precise MCD exception */ >> +#ifdef __aarch64__ >> +# define SEGV_MTEAERR 6 /* Asynchronous MTE error */ >> +# define SEGV_MTESERR 7 /* Synchronous MTE exception */ >> +#else >> +# define SEGV_ADIDERR 6 /* Disrupting MCD error */ >> +# define SEGV_ADIPERR 7 /* Precise MCD exception */ >> +#endif > > SEGV_ADIPERR/SEGV_ADIDERR were added together with SEGV_ACCADI, > it seems a bit odd to make only two of them conditional but not the others. > > I think we are generally working towards having the same constants > across architectures even for features that only exist on one of them. > > Adding Al and Eric to Cc, maybe they have another suggestion on what > constants should be used. > > Arnd
Hi Eric, On Thu, Dec 12, 2019 at 12:26:41PM -0600, Eric W. Biederman wrote: > Arnd Bergmann <arnd@arndb.de> writes: > > On Wed, Dec 11, 2019 at 7:40 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > >> > >> From: Vincenzo Frascino <vincenzo.frascino@arm.com> > >> > >> Add MTE-specific SIGSEGV codes to siginfo.h. > >> > >> Note that the for MTE we are reusing the same SPARC ADI codes because > >> the two functionalities are similar and they cannot coexist on the same > >> system. > > Please Please Please don't do that. > > It is actively harmful to have architecture specific si_code values. > As it makes maintenance much more difficult. > > Especially as the si_codes are part of union descrimanator. > > If your functionality is identical reuse the numbers otherwise please > just select the next numbers not yet used. It makes sense. > We have at least 256 si_codes per signal 2**32 if we really need them so > there is no need to be reuse numbers. > > The practical problem is that architecture specific si_codes start > turning kernel/signal.c into #ifdef soup, and we loose a lot of > basic compile coverage because of that. In turn not compiling the code > leads to bit-rot in all kinds of weird places. Fortunately for MTE we don't need to change kernel/signal.c. It's sufficient to call force_sig_fault() from the arch code with the corresponding signo, code and fault address. > p.s. As for coexistence there is always the possibility that one chip > in a cpu family does supports one thing and another chip in a cpu > family supports another. So userspace may have to cope with the > situation even if an individual chip doesn't. > > I remember a similar case where sparc had several distinct page table > formats and we had a single kernel that had to cope with them all. We have such fun on ARM as well with the big.LITTLE systems where not all CPUs support the same features. For example, MTE is only enabled once all the secondary CPUs have booted and confirmed to have the feature. Thanks.
Catalin Marinas <catalin.marinas@arm.com> writes: > Hi Eric, > > On Thu, Dec 12, 2019 at 12:26:41PM -0600, Eric W. Biederman wrote: >> Arnd Bergmann <arnd@arndb.de> writes: >> > On Wed, Dec 11, 2019 at 7:40 PM Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> >> >> From: Vincenzo Frascino <vincenzo.frascino@arm.com> >> >> >> >> Add MTE-specific SIGSEGV codes to siginfo.h. >> >> >> >> Note that the for MTE we are reusing the same SPARC ADI codes because >> >> the two functionalities are similar and they cannot coexist on the same >> >> system. >> >> Please Please Please don't do that. >> >> It is actively harmful to have architecture specific si_code values. >> As it makes maintenance much more difficult. >> >> Especially as the si_codes are part of union descrimanator. >> >> If your functionality is identical reuse the numbers otherwise please >> just select the next numbers not yet used. > > It makes sense. > >> We have at least 256 si_codes per signal 2**32 if we really need them so >> there is no need to be reuse numbers. >> >> The practical problem is that architecture specific si_codes start >> turning kernel/signal.c into #ifdef soup, and we loose a lot of >> basic compile coverage because of that. In turn not compiling the code >> leads to bit-rot in all kinds of weird places. > > Fortunately for MTE we don't need to change kernel/signal.c. It's > sufficient to call force_sig_fault() from the arch code with the > corresponding signo, code and fault address. Hooray for force_sig_fault at keeping people honest about which parameters they are passing. So far it looks like it is just BUS_MCEERR_AR, BUS_MCEERR_AO, SEGV_BNDERR, and SEGV_PKUERR that are the really confusing ones, as they go beyond the ordinary force_sig_fault layout. But we really do need the knowledge of how all of the cases are encoded or things can get very confusing. Especially when mixing 32bit and 64bit code. >> p.s. As for coexistence there is always the possibility that one chip >> in a cpu family does supports one thing and another chip in a cpu >> family supports another. So userspace may have to cope with the >> situation even if an individual chip doesn't. >> >> I remember a similar case where sparc had several distinct page table >> formats and we had a single kernel that had to cope with them all. > > We have such fun on ARM as well with the big.LITTLE systems where not > all CPUs support the same features. For example, MTE is only enabled > once all the secondary CPUs have booted and confirmed to have the > feature. Which all makes it possible that the alternative to MTE referenced as ADI might show up in some future ARM chip. Which really makes reusing the numbers a bad idea. Not that I actually recall what any of this functionality actually is, but I can tell when people are setting themselves of for a challenge unnecessarily. Eric
diff --git a/include/uapi/asm-generic/siginfo.h b/include/uapi/asm-generic/siginfo.h index cb3d6c267181..a5184a5438c6 100644 --- a/include/uapi/asm-generic/siginfo.h +++ b/include/uapi/asm-generic/siginfo.h @@ -227,8 +227,13 @@ typedef struct siginfo { # define SEGV_PKUERR 4 /* failed protection key checks */ #endif #define SEGV_ACCADI 5 /* ADI not enabled for mapped object */ -#define SEGV_ADIDERR 6 /* Disrupting MCD error */ -#define SEGV_ADIPERR 7 /* Precise MCD exception */ +#ifdef __aarch64__ +# define SEGV_MTEAERR 6 /* Asynchronous MTE error */ +# define SEGV_MTESERR 7 /* Synchronous MTE exception */ +#else +# define SEGV_ADIDERR 6 /* Disrupting MCD error */ +# define SEGV_ADIPERR 7 /* Precise MCD exception */ +#endif #define NSIGSEGV 7 /*