| Message ID | 20200515171612.1020-25-catalin.marinas@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: Memory Tagging Extension user-space support | expand |
On 5/15/20 6:16 PM, Catalin Marinas wrote: > For performance analysis it may be desirable to disable MTE altogether > via an early param. Introduce arm64.mte_disable and, if true, filter out > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > user. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > --- > > Notes: > New in v4. > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index f2a93c8679e8..7436e7462b85 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -373,6 +373,10 @@ > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > Format: <io>,<irq>,<nodeID> > > + arm64.mte_disable= > + [ARM64] Disable Linux support for the Memory > + Tagging Extension (both user and in-kernel). > + Should it really to take parameter (on/off/true/false)? It may lead to expectation that arm64.mte_disable=false should enable MT and, yes, double negatives make it look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? Cheers Vladimir
On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > For performance analysis it may be desirable to disable MTE altogether > > via an early param. Introduce arm64.mte_disable and, if true, filter out > > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > > user. > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > --- > > > > Notes: > > New in v4. > > > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index f2a93c8679e8..7436e7462b85 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -373,6 +373,10 @@ > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > Format: <io>,<irq>,<nodeID> > > > > + arm64.mte_disable= > > + [ARM64] Disable Linux support for the Memory > > + Tagging Extension (both user and in-kernel). > > + > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? I don't think "performance analysis" is a good justification for this parameter tbh. We don't tend to add these options for other architectural features, and I don't see why MTE is any different in this regard. Will
On Mon, May 18, 2020 at 12:31:03PM +0100, Will Deacon wrote: > On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > > For performance analysis it may be desirable to disable MTE altogether > > > via an early param. Introduce arm64.mte_disable and, if true, filter out > > > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > > > user. > > > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > Cc: Will Deacon <will@kernel.org> > > > --- > > > > > > Notes: > > > New in v4. > > > > > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > > > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > > > 2 files changed, 15 insertions(+) > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > index f2a93c8679e8..7436e7462b85 100644 > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > @@ -373,6 +373,10 @@ > > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > > Format: <io>,<irq>,<nodeID> > > > > > > + arm64.mte_disable= > > > + [ARM64] Disable Linux support for the Memory > > > + Tagging Extension (both user and in-kernel). > > > + > > > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? > > I don't think "performance analysis" is a good justification for this > parameter tbh. We don't tend to add these options for other architectural > features, and I don't see why MTE is any different in this regard. There is an expectation of performance impact with MTE enabled, especially if it's running in synchronous mode. For the in-kernel MTE, we could add a parameter which sets sync vs async at boot time rather than a big disable knob. It won't affect user space however. The other 'justification' is if your hardware has weird unexpected behaviour but I'd like this handled via errata workarounds. I'll let the people who asked for this to chip in ;). I agree with you that we rarely add these (and I rejected a similar option a few weeks ago on the AMU patchset).
On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > For performance analysis it may be desirable to disable MTE altogether > > via an early param. Introduce arm64.mte_disable and, if true, filter out > > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > > user. > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > --- > > > > Notes: > > New in v4. > > > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index f2a93c8679e8..7436e7462b85 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -373,6 +373,10 @@ > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > Format: <io>,<irq>,<nodeID> > > > > + arm64.mte_disable= > > + [ARM64] Disable Linux support for the Memory > > + Tagging Extension (both user and in-kernel). > > + > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? My reasoning about arm64.mte= was that 'on' may lead people to think it does something even when MTE isn't available on the SoC. So I ended up with an explicit 'disable' in the name. Happy to change it if we don't drop this parameter altogether (in the absence of valid use-cases).
On Mon, May 18, 2020 at 06:20:55PM +0100, Catalin Marinas wrote: > On Mon, May 18, 2020 at 12:31:03PM +0100, Will Deacon wrote: > > On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > > > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > > > For performance analysis it may be desirable to disable MTE altogether > > > > via an early param. Introduce arm64.mte_disable and, if true, filter out > > > > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > > > > user. > > > > > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > > > Cc: Will Deacon <will@kernel.org> > > > > --- > > > > > > > > Notes: > > > > New in v4. > > > > > > > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > > > > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > > > > 2 files changed, 15 insertions(+) > > > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > > index f2a93c8679e8..7436e7462b85 100644 > > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > > @@ -373,6 +373,10 @@ > > > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > > > Format: <io>,<irq>,<nodeID> > > > > > > > > + arm64.mte_disable= > > > > + [ARM64] Disable Linux support for the Memory > > > > + Tagging Extension (both user and in-kernel). > > > > + > > > > > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > > > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > > > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? > > > > I don't think "performance analysis" is a good justification for this > > parameter tbh. We don't tend to add these options for other architectural > > features, and I don't see why MTE is any different in this regard. > > There is an expectation of performance impact with MTE enabled, > especially if it's running in synchronous mode. For the in-kernel MTE, > we could add a parameter which sets sync vs async at boot time rather > than a big disable knob. It won't affect user space however. > > The other 'justification' is if your hardware has weird unexpected > behaviour but I'd like this handled via errata workarounds. > > I'll let the people who asked for this to chip in ;). I agree with you > that we rarely add these (and I rejected a similar option a few weeks > ago on the AMU patchset). We've been looking into other ways this on/off behavior could be achieved. The "arm,armv8.5-memtag" DT flag already provides what we want - meaning that this flag could be removed if the system did not support MTE. I did see your remark on "arm64: mte: Check the DT memory nodes for MTE support" questioning whether it was the right approach - is this still the case? --Patrick
Hi Patrick, On Thu, May 21, 2020 at 10:57:10PM -0700, Patrick Daly wrote: > On Mon, May 18, 2020 at 06:20:55PM +0100, Catalin Marinas wrote: > > On Mon, May 18, 2020 at 12:31:03PM +0100, Will Deacon wrote: > > > On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > > > > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > > > index f2a93c8679e8..7436e7462b85 100644 > > > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > > > @@ -373,6 +373,10 @@ > > > > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > > > > Format: <io>,<irq>,<nodeID> > > > > > > > > > > + arm64.mte_disable= > > > > > + [ARM64] Disable Linux support for the Memory > > > > > + Tagging Extension (both user and in-kernel). > > > > > + > > > > > > > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > > > > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > > > > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? > > > > > > I don't think "performance analysis" is a good justification for this > > > parameter tbh. We don't tend to add these options for other architectural > > > features, and I don't see why MTE is any different in this regard. > > > > There is an expectation of performance impact with MTE enabled, > > especially if it's running in synchronous mode. For the in-kernel MTE, > > we could add a parameter which sets sync vs async at boot time rather > > than a big disable knob. It won't affect user space however. > > > > The other 'justification' is if your hardware has weird unexpected > > behaviour but I'd like this handled via errata workarounds. > > > > I'll let the people who asked for this to chip in ;). I agree with you > > that we rarely add these (and I rejected a similar option a few weeks > > ago on the AMU patchset). > > We've been looking into other ways this on/off behavior could be achieved. The actual question here is what the on/off behaviour is needed for. We can figure out the best mechanism for this once we know what we want to achieve. My wild guess above was performance analysis but that can be toggled by either kernel boot parameter or run-time sysctl (or just the Kconfig option). If it is about forcing user space not to use MTE, we may look into some other sysctl controls (we already have one for the tagged address ABI). If it is for working around hardware not supporting MTE (i.e. no allocation tag storage), this should be handled differently, not by kernel parameter. > The "arm,armv8.5-memtag" DT flag already provides what we want - meaning > that this flag could be removed if the system did not support MTE. > > I did see your remark on "arm64: mte: Check the DT memory nodes for MTE support" > questioning whether it was the right approach - is this still the case? My plan is to remove the DT patch altogether _if_ I get confirmation from the CPU designers. The idea is that if ID_AA64PFR1_EL1.MTE > 1, Linux can assume system-wide MTE support. If an MTE-capable CPU is deployed in an SoC without tag storage, a tie-off should change the ID field to 1 (or 0). If we do find hardware with an ID field > 1 and no tag storage, it will be handled as an SoC erratum in the kernel, probably tied to the new SoC Id advertised by firmware (Sudeep had some patches recently). Thanks.
On Fri, May 22, 2020 at 11:37:15AM +0100, Catalin Marinas wrote: > Hi Patrick, > > On Thu, May 21, 2020 at 10:57:10PM -0700, Patrick Daly wrote: > > On Mon, May 18, 2020 at 06:20:55PM +0100, Catalin Marinas wrote: > > > On Mon, May 18, 2020 at 12:31:03PM +0100, Will Deacon wrote: > > > > On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > > > > > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > > > > index f2a93c8679e8..7436e7462b85 100644 > > > > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > > > > @@ -373,6 +373,10 @@ > > > > > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > > > > > Format: <io>,<irq>,<nodeID> > > > > > > > > > > > > + arm64.mte_disable= > > > > > > + [ARM64] Disable Linux support for the Memory > > > > > > + Tagging Extension (both user and in-kernel). > > > > > > + > > > > > > > > > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > > > > > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > > > > > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? > > > > > > > > I don't think "performance analysis" is a good justification for this > > > > parameter tbh. We don't tend to add these options for other architectural > > > > features, and I don't see why MTE is any different in this regard. > > > > > > There is an expectation of performance impact with MTE enabled, > > > especially if it's running in synchronous mode. For the in-kernel MTE, > > > we could add a parameter which sets sync vs async at boot time rather > > > than a big disable knob. It won't affect user space however. > > > > > > The other 'justification' is if your hardware has weird unexpected > > > behaviour but I'd like this handled via errata workarounds. > > > > > > I'll let the people who asked for this to chip in ;). I agree with you > > > that we rarely add these (and I rejected a similar option a few weeks > > > ago on the AMU patchset). > > > > We've been looking into other ways this on/off behavior could be achieved. > > The actual question here is what the on/off behaviour is needed for. We > can figure out the best mechanism for this once we know what we want to > achieve. My wild guess above was performance analysis but that can be > toggled by either kernel boot parameter or run-time sysctl (or just the > Kconfig option). > > If it is about forcing user space not to use MTE, we may look into some > other sysctl controls (we already have one for the tagged address ABI). We want to allow the end user to be able to easily "opt out" of MTE in favour of better power, perf and battery life. In terms of deciding policy, a sysctl is much more accessible than reompiling with CONFIG_MTE=n, or replacing userspace libraries with equivalents which don't use PROT_MTE. --Patrick > > If it is for working around hardware not supporting MTE (i.e. no > allocation tag storage), this should be handled differently, not by > kernel parameter. > > > The "arm,armv8.5-memtag" DT flag already provides what we want - meaning > > that this flag could be removed if the system did not support MTE. > > > > I did see your remark on "arm64: mte: Check the DT memory nodes for MTE support" > > questioning whether it was the right approach - is this still the case? > > My plan is to remove the DT patch altogether _if_ I get confirmation > from the CPU designers. The idea is that if ID_AA64PFR1_EL1.MTE > 1, > Linux can assume system-wide MTE support. If an MTE-capable CPU is > deployed in an SoC without tag storage, a tie-off should change the ID > field to 1 (or 0). If we do find hardware with an ID field > 1 and no > tag storage, it will be handled as an SoC erratum in the kernel, > probably tied to the new SoC Id advertised by firmware (Sudeep had some > patches recently).
On Tue, May 26, 2020 at 07:11:53PM -0700, Patrick Daly wrote: > On Fri, May 22, 2020 at 11:37:15AM +0100, Catalin Marinas wrote: > > On Thu, May 21, 2020 at 10:57:10PM -0700, Patrick Daly wrote: > > > On Mon, May 18, 2020 at 06:20:55PM +0100, Catalin Marinas wrote: > > > > On Mon, May 18, 2020 at 12:31:03PM +0100, Will Deacon wrote: > > > > > On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > > > > > > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > > > > > index f2a93c8679e8..7436e7462b85 100644 > > > > > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > > > > > @@ -373,6 +373,10 @@ > > > > > > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > > > > > > Format: <io>,<irq>,<nodeID> > > > > > > > > > > > > > > + arm64.mte_disable= > > > > > > > + [ARM64] Disable Linux support for the Memory > > > > > > > + Tagging Extension (both user and in-kernel). > > > > > > > + > > > > > > > > > > > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > > > > > > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > > > > > > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? > > > > > > > > > > I don't think "performance analysis" is a good justification for this > > > > > parameter tbh. We don't tend to add these options for other architectural > > > > > features, and I don't see why MTE is any different in this regard. > > > > > > > > There is an expectation of performance impact with MTE enabled, > > > > especially if it's running in synchronous mode. For the in-kernel MTE, > > > > we could add a parameter which sets sync vs async at boot time rather > > > > than a big disable knob. It won't affect user space however. > > > > > > > > The other 'justification' is if your hardware has weird unexpected > > > > behaviour but I'd like this handled via errata workarounds. > > > > > > > > I'll let the people who asked for this to chip in ;). I agree with you > > > > that we rarely add these (and I rejected a similar option a few weeks > > > > ago on the AMU patchset). > > > > > > We've been looking into other ways this on/off behavior could be achieved. > > > > The actual question here is what the on/off behaviour is needed for. We > > can figure out the best mechanism for this once we know what we want to > > achieve. My wild guess above was performance analysis but that can be > > toggled by either kernel boot parameter or run-time sysctl (or just the > > Kconfig option). > > > > If it is about forcing user space not to use MTE, we may look into some > > other sysctl controls (we already have one for the tagged address ABI). > > We want to allow the end user to be able to easily "opt out" of MTE in favour > of better power, perf and battery life. Who is "the end user" in this case? If MTE is bad enough for power, performance and battery life that we need a kill switch, then perhaps we shouldn't enable it by default and the few people that want to use it can build a kernel with it enabled. However, then I don't really see what MTE buys you over the existing KASAN implementations. I thought the general idea was that you could run in the (cheap) "async" mode, and then re-run in the more expensive "sync" mode to further diagnose any failures. That model seems to work well with these patches, since reporting is disabled by default. Are you saying that there is a significant penalty incurred even when reporting is not enabled? Anyway, we don't offer global runtime/cmdline switches for the vast majority of other architectural features -- instead, we choose a sensible default, and I think we should do the same here. Will
The 05/27/2020 10:55, Will Deacon wrote: > On Tue, May 26, 2020 at 07:11:53PM -0700, Patrick Daly wrote: > > On Fri, May 22, 2020 at 11:37:15AM +0100, Catalin Marinas wrote: > > > The actual question here is what the on/off behaviour is needed for. We > > > can figure out the best mechanism for this once we know what we want to > > > achieve. My wild guess above was performance analysis but that can be > > > toggled by either kernel boot parameter or run-time sysctl (or just the > > > Kconfig option). > > > > > > If it is about forcing user space not to use MTE, we may look into some > > > other sysctl controls (we already have one for the tagged address ABI). > > > > We want to allow the end user to be able to easily "opt out" of MTE in favour > > of better power, perf and battery life. > > Who is "the end user" in this case? > > If MTE is bad enough for power, performance and battery life that we need a > kill switch, then perhaps we shouldn't enable it by default and the few > people that want to use it can build a kernel with it enabled. However, then > I don't really see what MTE buys you over the existing KASAN implementations. > > I thought the general idea was that you could run in the (cheap) "async" > mode, and then re-run in the more expensive "sync" mode to further diagnose > any failures. That model seems to work well with these patches, since > reporting is disabled by default. Are you saying that there is a > significant penalty incurred even when reporting is not enabled? > > Anyway, we don't offer global runtime/cmdline switches for the vast majority > of other architectural features -- instead, we choose a sensible default, > and I think we should do the same here. i would not expect mte overhead if userspace processes don't map anything with PROT_MTE and don't enable tag checking with prctl. (i.e. userspace runtimes can "opt out" of mte for better power, perf and battery) is that not the case?
On Wed, May 27, 2020 at 10:55:05AM +0100, Will Deacon wrote: > On Tue, May 26, 2020 at 07:11:53PM -0700, Patrick Daly wrote: > > On Fri, May 22, 2020 at 11:37:15AM +0100, Catalin Marinas wrote: > > > On Thu, May 21, 2020 at 10:57:10PM -0700, Patrick Daly wrote: > > > > On Mon, May 18, 2020 at 06:20:55PM +0100, Catalin Marinas wrote: > > > > > On Mon, May 18, 2020 at 12:31:03PM +0100, Will Deacon wrote: > > > > > > On Mon, May 18, 2020 at 12:26:30PM +0100, Vladimir Murzin wrote: > > > > > > > On 5/15/20 6:16 PM, Catalin Marinas wrote: > > > > > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > > > > > > > index f2a93c8679e8..7436e7462b85 100644 > > > > > > > > --- a/Documentation/admin-guide/kernel-parameters.txt > > > > > > > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > > > > > > > @@ -373,6 +373,10 @@ > > > > > > > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > > > > > > > Format: <io>,<irq>,<nodeID> > > > > > > > > > > > > > > > > + arm64.mte_disable= > > > > > > > > + [ARM64] Disable Linux support for the Memory > > > > > > > > + Tagging Extension (both user and in-kernel). > > > > > > > > + > > > > > > > > > > > > > > Should it really to take parameter (on/off/true/false)? It may lead to expectation > > > > > > > that arm64.mte_disable=false should enable MT and, yes, double negatives make it > > > > > > > look ugly, so if we do need parameter, can it be arm64.mte=on/off/true/false? > > > > > > > > > > > > I don't think "performance analysis" is a good justification for this > > > > > > parameter tbh. We don't tend to add these options for other architectural > > > > > > features, and I don't see why MTE is any different in this regard. > > > > > > > > > > There is an expectation of performance impact with MTE enabled, > > > > > especially if it's running in synchronous mode. For the in-kernel MTE, > > > > > we could add a parameter which sets sync vs async at boot time rather > > > > > than a big disable knob. It won't affect user space however. > > > > > > > > > > The other 'justification' is if your hardware has weird unexpected > > > > > behaviour but I'd like this handled via errata workarounds. > > > > > > > > > > I'll let the people who asked for this to chip in ;). I agree with you > > > > > that we rarely add these (and I rejected a similar option a few weeks > > > > > ago on the AMU patchset). > > > > > > > > We've been looking into other ways this on/off behavior could be achieved. > > > > > > The actual question here is what the on/off behaviour is needed for. We > > > can figure out the best mechanism for this once we know what we want to > > > achieve. My wild guess above was performance analysis but that can be > > > toggled by either kernel boot parameter or run-time sysctl (or just the > > > Kconfig option). > > > > > > If it is about forcing user space not to use MTE, we may look into some > > > other sysctl controls (we already have one for the tagged address ABI). > > > > We want to allow the end user to be able to easily "opt out" of MTE in favour > > of better power, perf and battery life. > > Who is "the end user" in this case? Good question. I have a suspicion it's still the (kernel) developer ;). > If MTE is bad enough for power, performance and battery life that we need a > kill switch, then perhaps we shouldn't enable it by default and the few > people that want to use it can build a kernel with it enabled. However, then > I don't really see what MTE buys you over the existing KASAN implementations. MTE is faster than KASan (with async mode the fastest), however I'd expect it to still be noticeable compared to no-MTE. It's a trade-off if you want to find security bugs in your code on a large scale. > I thought the general idea was that you could run in the (cheap) "async" > mode, and then re-run in the more expensive "sync" mode to further diagnose > any failures. That model seems to work well with these patches, since > reporting is disabled by default. Are you saying that there is a > significant penalty incurred even when reporting is not enabled? The tag checking mode is controlled by the user on a per-process basis. The modes and hardware perf/power expectations: 1. no tag checking - no expected performance penalty from the hardware perspective (tags not fetched from memory). 2. async tag checking - tags fetched from memory but checked asynchronously, so it allows the hardware to perform as well as it can (I don't have numbers yet). Probably a small degradation vs (1). 3. sync tag checking - there is an expectation of further perf/power degradation vs (2). In addition to the hardware aspects above, you have the software cost for colouring memory both on allocation and on free. By default, a malloc()/free() wouldn't touch the memory (maybe some red zones) but with MTE the libc will have to set the colour. That's faster than a memset since it need to store 4 bits for every 16 bytes of address but slower than not doing it at all. For a calloc(), The memset + tag setting can be combined in a single DC instruction. So, it really depends on what the user is doing. If we want a knob where the user doesn't even attempt to colour pages (not even (1) above), maybe a user space env variable parsed by the libc is a better option. While MTE and the tagged addr ABI are complementary (one can still set PROT_MTE without enabling the tagged addr ABI), most likely a libc implementation would try to enable the latter before using MTE. We already have a sysctl to force the tagged addr ABI off. The side-effect is that MTE will be disabled in the C library, so assuming no run-time cost (the libc people to confirm). The tagged addr sysctl doesn't cover the in-kernel MTE but we can leave the discussion for when we have the patches. > Anyway, we don't offer global runtime/cmdline switches for the vast majority > of other architectural features -- instead, we choose a sensible default, > and I think we should do the same here. The sensible defaults are currently "off" with a user opt-in. I think the question is whether we need a "safety" knob at the kernel level like we did with the sysctl abi.tagged_addr_disabled or we leave it to the user as it sees fit (e.g. env variables) since it doesn't affect the kernel (unlike the tagged addr ABI).
On Fri, May 15, 2020 at 7:17 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > For performance analysis it may be desirable to disable MTE altogether > via an early param. Introduce arm64.mte_disable and, if true, filter out > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > user. > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > Cc: Will Deacon <will@kernel.org> > --- > > Notes: > New in v4. > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > index f2a93c8679e8..7436e7462b85 100644 > --- a/Documentation/admin-guide/kernel-parameters.txt > +++ b/Documentation/admin-guide/kernel-parameters.txt > @@ -373,6 +373,10 @@ > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > Format: <io>,<irq>,<nodeID> > > + arm64.mte_disable= > + [ARM64] Disable Linux support for the Memory > + Tagging Extension (both user and in-kernel). > + > ataflop= [HW,M68k] > > atarimouse= [HW,MOUSE] Atari Mouse > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > index aaadc1cbc006..f7596830694f 100644 > --- a/arch/arm64/kernel/cpufeature.c > +++ b/arch/arm64/kernel/cpufeature.c > @@ -126,12 +126,23 @@ static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); > static bool __system_matches_cap(unsigned int n); > > #ifdef CONFIG_ARM64_MTE > +static bool mte_disable; > + > +static int __init arm64_mte_disable(char *buf) > +{ > + return strtobool(buf, &mte_disable); > +} > +early_param("arm64.mte_disable", arm64_mte_disable); > + > s64 mte_ftr_filter(const struct arm64_ftr_bits *ftrp, s64 val) > { > struct device_node *np; > static bool memory_checked = false; > static bool mte_capable = true; > > + if (mte_disable) > + return ID_AA64PFR1_MTE_NI; > + > /* EL0-only MTE is not supported by Linux, don't expose it */ > if (val < ID_AA64PFR1_MTE) > return ID_AA64PFR1_MTE_NI; Hi Calatin, While this patch didn't land upstream, we need an MTE kill-switch for Android GKI. Is this patch OK to take as is? Is it still valid? Thanks!
On Thu, Jan 21, 2021 at 8:37 PM Andrey Konovalov <andreyknvl@google.com> wrote: > > On Fri, May 15, 2020 at 7:17 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > > For performance analysis it may be desirable to disable MTE altogether > > via an early param. Introduce arm64.mte_disable and, if true, filter out > > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > > user. > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > --- > > > > Notes: > > New in v4. > > > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index f2a93c8679e8..7436e7462b85 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -373,6 +373,10 @@ > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > Format: <io>,<irq>,<nodeID> > > > > + arm64.mte_disable= > > + [ARM64] Disable Linux support for the Memory > > + Tagging Extension (both user and in-kernel). > > + > > ataflop= [HW,M68k] > > > > atarimouse= [HW,MOUSE] Atari Mouse > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index aaadc1cbc006..f7596830694f 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -126,12 +126,23 @@ static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); > > static bool __system_matches_cap(unsigned int n); > > > > #ifdef CONFIG_ARM64_MTE > > +static bool mte_disable; > > + > > +static int __init arm64_mte_disable(char *buf) > > +{ > > + return strtobool(buf, &mte_disable); > > +} > > +early_param("arm64.mte_disable", arm64_mte_disable); > > + > > s64 mte_ftr_filter(const struct arm64_ftr_bits *ftrp, s64 val) > > { > > struct device_node *np; > > static bool memory_checked = false; > > static bool mte_capable = true; > > > > + if (mte_disable) > > + return ID_AA64PFR1_MTE_NI; > > + > > /* EL0-only MTE is not supported by Linux, don't expose it */ > > if (val < ID_AA64PFR1_MTE) > > return ID_AA64PFR1_MTE_NI; > > Hi Catalin, > > While this patch didn't land upstream, we need an MTE kill-switch for > Android GKI. Is this patch OK to take as is? Is it still valid? Looking at this more closely: looks like this code no longer exists. What would be the approach to add this kind of switch now? Thanks!
On Thu, Jan 21, 2021 at 08:37:18PM +0100, Andrey Konovalov wrote: > On Fri, May 15, 2020 at 7:17 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > For performance analysis it may be desirable to disable MTE altogether > > via an early param. Introduce arm64.mte_disable and, if true, filter out > > the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to > > user. > > > > Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> > > Cc: Will Deacon <will@kernel.org> > > --- > > > > Notes: > > New in v4. > > > > Documentation/admin-guide/kernel-parameters.txt | 4 ++++ > > arch/arm64/kernel/cpufeature.c | 11 +++++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt > > index f2a93c8679e8..7436e7462b85 100644 > > --- a/Documentation/admin-guide/kernel-parameters.txt > > +++ b/Documentation/admin-guide/kernel-parameters.txt > > @@ -373,6 +373,10 @@ > > arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards > > Format: <io>,<irq>,<nodeID> > > > > + arm64.mte_disable= > > + [ARM64] Disable Linux support for the Memory > > + Tagging Extension (both user and in-kernel). > > + > > ataflop= [HW,M68k] > > > > atarimouse= [HW,MOUSE] Atari Mouse > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c > > index aaadc1cbc006..f7596830694f 100644 > > --- a/arch/arm64/kernel/cpufeature.c > > +++ b/arch/arm64/kernel/cpufeature.c > > @@ -126,12 +126,23 @@ static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); > > static bool __system_matches_cap(unsigned int n); > > > > #ifdef CONFIG_ARM64_MTE > > +static bool mte_disable; > > + > > +static int __init arm64_mte_disable(char *buf) > > +{ > > + return strtobool(buf, &mte_disable); > > +} > > +early_param("arm64.mte_disable", arm64_mte_disable); > > + > > s64 mte_ftr_filter(const struct arm64_ftr_bits *ftrp, s64 val) > > { > > struct device_node *np; > > static bool memory_checked = false; > > static bool mte_capable = true; > > > > + if (mte_disable) > > + return ID_AA64PFR1_MTE_NI; > > + > > /* EL0-only MTE is not supported by Linux, don't expose it */ > > if (val < ID_AA64PFR1_MTE) > > return ID_AA64PFR1_MTE_NI; > > While this patch didn't land upstream, we need an MTE kill-switch for > Android GKI. Is this patch OK to take as is? Is it still valid? As you noticed, this code no longer exists. The CPUID is checked early during boot in proc.S, before the MMU is enabled, as you need to set up the MAIR register. Now, what do you mean by kill switch? There are multiple levels at which one can disable MTE or some of its effects: memory type (MAIR) level, tag allocation (TCR_EL1.ATA), tag checking (SCTLR_EL1.TCF). Apart from the latter, all the other bits are cached in the TLB which make them more problematic to toggle at run-time. For the kernel, we can currently disable tag checking via the kasan command line options. For user-space, we don't have a kill switch specific to MTE, however one can disable the tagged addr ABI and presumably the C library will avoid generating tagged heap pointers.
On Fri, Jan 22, 2021 at 3:41 PM Catalin Marinas <catalin.marinas@arm.com> wrote: > > > While this patch didn't land upstream, we need an MTE kill-switch for > > Android GKI. Is this patch OK to take as is? Is it still valid? > > As you noticed, this code no longer exists. The CPUID is checked early > during boot in proc.S, before the MMU is enabled, as you need to set up > the MAIR register. > > Now, what do you mean by kill switch? There are multiple levels at which > one can disable MTE or some of its effects: memory type (MAIR) level, > tag allocation (TCR_EL1.ATA), tag checking (SCTLR_EL1.TCF). Apart from > the latter, all the other bits are cached in the TLB which make them > more problematic to toggle at run-time. > > For the kernel, we can currently disable tag checking via the kasan > command line options. For user-space, we don't have a kill switch > specific to MTE, however one can disable the tagged addr ABI and > presumably the C library will avoid generating tagged heap pointers. Just FTR: As discussed off-the-list, there won't be any need for a kill-switch for userspace MTE. Thanks!
diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt index f2a93c8679e8..7436e7462b85 100644 --- a/Documentation/admin-guide/kernel-parameters.txt +++ b/Documentation/admin-guide/kernel-parameters.txt @@ -373,6 +373,10 @@ arcrimi= [HW,NET] ARCnet - "RIM I" (entirely mem-mapped) cards Format: <io>,<irq>,<nodeID> + arm64.mte_disable= + [ARM64] Disable Linux support for the Memory + Tagging Extension (both user and in-kernel). + ataflop= [HW,M68k] atarimouse= [HW,MOUSE] Atari Mouse diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c index aaadc1cbc006..f7596830694f 100644 --- a/arch/arm64/kernel/cpufeature.c +++ b/arch/arm64/kernel/cpufeature.c @@ -126,12 +126,23 @@ static void cpu_enable_cnp(struct arm64_cpu_capabilities const *cap); static bool __system_matches_cap(unsigned int n); #ifdef CONFIG_ARM64_MTE +static bool mte_disable; + +static int __init arm64_mte_disable(char *buf) +{ + return strtobool(buf, &mte_disable); +} +early_param("arm64.mte_disable", arm64_mte_disable); + s64 mte_ftr_filter(const struct arm64_ftr_bits *ftrp, s64 val) { struct device_node *np; static bool memory_checked = false; static bool mte_capable = true; + if (mte_disable) + return ID_AA64PFR1_MTE_NI; + /* EL0-only MTE is not supported by Linux, don't expose it */ if (val < ID_AA64PFR1_MTE) return ID_AA64PFR1_MTE_NI;
For performance analysis it may be desirable to disable MTE altogether via an early param. Introduce arm64.mte_disable and, if true, filter out the sanitised ID_AA64PFR1_EL1.MTE field to avoid exposing the HWCAP to user. Signed-off-by: Catalin Marinas <catalin.marinas@arm.com> Cc: Will Deacon <will@kernel.org> --- Notes: New in v4. Documentation/admin-guide/kernel-parameters.txt | 4 ++++ arch/arm64/kernel/cpufeature.c | 11 +++++++++++ 2 files changed, 15 insertions(+)