Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1570676 > unrolled thread

Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Mark Rutland <mark.rutland@arm.com> - 2017-01-31 13:40 +0100
    Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Christopher Covington <cov@codeaurora.org> - 2017-01-31 18:50 +0100
      Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Marc Zyngier <marc.zyngier@arm.com> - 2017-01-31 19:00 +0100
        Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Christopher Covington <cov@codeaurora.org> - 2017-02-01 17:30 +0100
          Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Will Deacon <will.deacon@arm.com> - 2017-02-01 17:40 +0100
            Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Catalin Marinas <catalin.marinas@arm.com> - 2017-02-01 18:40 +0100
              Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Will Deacon <will.deacon@arm.com> - 2017-02-01 18:50 +0100
                Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Will Deacon <will.deacon@arm.com> - 2017-02-01 19:00 +0100
                  Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Catalin Marinas <catalin.marinas@arm.com> - 2017-02-01 19:30 +0100
                    Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Will Deacon <will.deacon@arm.com> - 2017-02-01 19:40 +0100
                      Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Catalin Marinas <catalin.marinas@arm.com> - 2017-02-01 19:40 +0100
                        Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Christopher Covington <cov@codeaurora.org> - 2017-02-08 01:40 +0100
                Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Catalin Marinas <catalin.marinas@arm.com> - 2017-02-01 19:00 +0100
                Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003 Catalin Marinas <catalin.marinas@arm.com> - 2017-02-01 19:00 +0100

#1570676 — Re: [PATCH v4 2/4] arm64: Work around Falkor erratum 1003

On Wed, Jan 25, 2017 at 10:52:30AM -0500, Christopher Covington wrote:
> The Qualcomm Datacenter Technologies Falkor v1 CPU may allocate TLB entries
> using an incorrect ASID when TTBRx_EL1 is being updated. When the erratum
> is triggered, page table entries using the new translation table base
> address (BADDR) will be allocated into the TLB using the old ASID. All
> circumstances leading to the incorrect ASID being cached in the TLB arise
> when software writes TTBRx_EL1[ASID] and TTBRx_EL1[BADDR], a memory
> operation is in the process of performing a translation using the specific
> TTBRx_EL1 being written, and the memory operation uses a translation table
> descriptor designated as non-global. EL2 and EL3 code changing the EL1&0
> ASID is not subject to this erratum because hardware is prohibited from
> performing translations from an out-of-context translation regime.
> 
> Consider the following pseudo code.
> 
>   write new BADDR and ASID values to TTBRx_EL1
> 
> Replacing the above sequence with the one below will ensure that no TLB
> entries with an incorrect ASID are used by software.
> 
>   write reserved value to TTBRx_EL1[ASID]
>   ISB
>   write new value to TTBRx_EL1[BADDR]
>   ISB
>   write new value to TTBRx_EL1[ASID]
>   ISB
> 
> When the above sequence is used, page table entries using the new BADDR
> value may still be incorrectly allocated into the TLB using the reserved
> ASID. Yet this will not reduce functionality, since TLB entries incorrectly
> tagged with the reserved ASID will never be hit by a later instruction.

Based on my understanding that entries allocated to the reserved ASID
will not be used for subsequent page table walks (and so we don't have
asynchronous behaviour to contend with), this sounds fine to me.

Thanks for taking the time to clarify the details on that.

> Based on work by Shanker Donthineni <shankerd@codeaurora.org>
> 
> Signed-off-by: Christopher Covington <cov@codeaurora.org>
> ---
>  Documentation/arm64/silicon-errata.txt |  1 +
>  arch/arm64/Kconfig                     | 11 +++++++++++
>  arch/arm64/include/asm/assembler.h     | 23 +++++++++++++++++++++++
>  arch/arm64/include/asm/cpucaps.h       |  3 ++-
>  arch/arm64/include/asm/mmu_context.h   |  8 +++++++-
>  arch/arm64/kernel/cpu_errata.c         |  7 +++++++
>  arch/arm64/mm/context.c                | 11 +++++++++++
>  arch/arm64/mm/proc.S                   |  1 +
>  8 files changed, 63 insertions(+), 2 deletions(-)

Don't we need to use pre_ttbr0_update_workaround in <asm/asm-uaccess.h>
for CONFIG_ARM64_SW_TTBR0_PAN? We implicitly switch to the reserved ASID
for the empty table in __uaccess_ttbr0_disable.

That also means we have to invalidate the reserved ASID so as to not
accidentally hit while uaccess is disabled.

Thanks,
Mark.

[toc] | [next] | [standalone]

#1570935

On 01/31/2017 07:37 AM, Mark Rutland wrote:
> On Wed, Jan 25, 2017 at 10:52:30AM -0500, Christopher Covington wrote:
>> The Qualcomm Datacenter Technologies Falkor v1 CPU may allocate TLB entries
>> using an incorrect ASID when TTBRx_EL1 is being updated. When the erratum
>> is triggered, page table entries using the new translation table base
>> address (BADDR) will be allocated into the TLB using the old ASID. All
>> circumstances leading to the incorrect ASID being cached in the TLB arise
>> when software writes TTBRx_EL1[ASID] and TTBRx_EL1[BADDR], a memory
>> operation is in the process of performing a translation using the specific
>> TTBRx_EL1 being written, and the memory operation uses a translation table
>> descriptor designated as non-global. EL2 and EL3 code changing the EL1&0
>> ASID is not subject to this erratum because hardware is prohibited from
>> performing translations from an out-of-context translation regime.
>>
>> Consider the following pseudo code.
>>
>>   write new BADDR and ASID values to TTBRx_EL1
>>
>> Replacing the above sequence with the one below will ensure that no TLB
>> entries with an incorrect ASID are used by software.
>>
>>   write reserved value to TTBRx_EL1[ASID]
>>   ISB
>>   write new value to TTBRx_EL1[BADDR]
>>   ISB
>>   write new value to TTBRx_EL1[ASID]
>>   ISB
>>
>> When the above sequence is used, page table entries using the new BADDR
>> value may still be incorrectly allocated into the TLB using the reserved
>> ASID. Yet this will not reduce functionality, since TLB entries incorrectly
>> tagged with the reserved ASID will never be hit by a later instruction.
> 
> Based on my understanding that entries allocated to the reserved ASID
> will not be used for subsequent page table walks (and so we don't have
> asynchronous behaviour to contend with), this sounds fine to me.
> 
> Thanks for taking the time to clarify the details on that.
> 
>> Based on work by Shanker Donthineni <shankerd@codeaurora.org>
>>
>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>> ---
>>  Documentation/arm64/silicon-errata.txt |  1 +
>>  arch/arm64/Kconfig                     | 11 +++++++++++
>>  arch/arm64/include/asm/assembler.h     | 23 +++++++++++++++++++++++
>>  arch/arm64/include/asm/cpucaps.h       |  3 ++-
>>  arch/arm64/include/asm/mmu_context.h   |  8 +++++++-
>>  arch/arm64/kernel/cpu_errata.c         |  7 +++++++
>>  arch/arm64/mm/context.c                | 11 +++++++++++
>>  arch/arm64/mm/proc.S                   |  1 +
>>  8 files changed, 63 insertions(+), 2 deletions(-)
> 
> Don't we need to use pre_ttbr0_update_workaround in <asm/asm-uaccess.h>
> for CONFIG_ARM64_SW_TTBR0_PAN? We implicitly switch to the reserved ASID
> for the empty table in __uaccess_ttbr0_disable.
> 
> That also means we have to invalidate the reserved ASID so as to not
> accidentally hit while uaccess is disabled.

The CPU in question (Falkor v1) has hardware PAN support. Do we need
to worry about including the workaround in the SW PAN code in that case?

Thanks,
Cov

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1570944

On 31/01/17 17:48, Christopher Covington wrote:
> On 01/31/2017 07:37 AM, Mark Rutland wrote:
>> On Wed, Jan 25, 2017 at 10:52:30AM -0500, Christopher Covington wrote:
>>> The Qualcomm Datacenter Technologies Falkor v1 CPU may allocate TLB entries
>>> using an incorrect ASID when TTBRx_EL1 is being updated. When the erratum
>>> is triggered, page table entries using the new translation table base
>>> address (BADDR) will be allocated into the TLB using the old ASID. All
>>> circumstances leading to the incorrect ASID being cached in the TLB arise
>>> when software writes TTBRx_EL1[ASID] and TTBRx_EL1[BADDR], a memory
>>> operation is in the process of performing a translation using the specific
>>> TTBRx_EL1 being written, and the memory operation uses a translation table
>>> descriptor designated as non-global. EL2 and EL3 code changing the EL1&0
>>> ASID is not subject to this erratum because hardware is prohibited from
>>> performing translations from an out-of-context translation regime.
>>>
>>> Consider the following pseudo code.
>>>
>>>   write new BADDR and ASID values to TTBRx_EL1
>>>
>>> Replacing the above sequence with the one below will ensure that no TLB
>>> entries with an incorrect ASID are used by software.
>>>
>>>   write reserved value to TTBRx_EL1[ASID]
>>>   ISB
>>>   write new value to TTBRx_EL1[BADDR]
>>>   ISB
>>>   write new value to TTBRx_EL1[ASID]
>>>   ISB
>>>
>>> When the above sequence is used, page table entries using the new BADDR
>>> value may still be incorrectly allocated into the TLB using the reserved
>>> ASID. Yet this will not reduce functionality, since TLB entries incorrectly
>>> tagged with the reserved ASID will never be hit by a later instruction.
>>
>> Based on my understanding that entries allocated to the reserved ASID
>> will not be used for subsequent page table walks (and so we don't have
>> asynchronous behaviour to contend with), this sounds fine to me.
>>
>> Thanks for taking the time to clarify the details on that.
>>
>>> Based on work by Shanker Donthineni <shankerd@codeaurora.org>
>>>
>>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>> ---
>>>  Documentation/arm64/silicon-errata.txt |  1 +
>>>  arch/arm64/Kconfig                     | 11 +++++++++++
>>>  arch/arm64/include/asm/assembler.h     | 23 +++++++++++++++++++++++
>>>  arch/arm64/include/asm/cpucaps.h       |  3 ++-
>>>  arch/arm64/include/asm/mmu_context.h   |  8 +++++++-
>>>  arch/arm64/kernel/cpu_errata.c         |  7 +++++++
>>>  arch/arm64/mm/context.c                | 11 +++++++++++
>>>  arch/arm64/mm/proc.S                   |  1 +
>>>  8 files changed, 63 insertions(+), 2 deletions(-)
>>
>> Don't we need to use pre_ttbr0_update_workaround in <asm/asm-uaccess.h>
>> for CONFIG_ARM64_SW_TTBR0_PAN? We implicitly switch to the reserved ASID
>> for the empty table in __uaccess_ttbr0_disable.
>>
>> That also means we have to invalidate the reserved ASID so as to not
>> accidentally hit while uaccess is disabled.
> 
> The CPU in question (Falkor v1) has hardware PAN support. Do we need
> to worry about including the workaround in the SW PAN code in that case?

Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
that case too, and hope that people do enable the HW version.

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

[toc] | [prev] | [next] | [standalone]

#1571703

On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> On 31/01/17 17:48, Christopher Covington wrote:
>> On 01/31/2017 07:37 AM, Mark Rutland wrote:
>>> On Wed, Jan 25, 2017 at 10:52:30AM -0500, Christopher Covington wrote:
>>>> The Qualcomm Datacenter Technologies Falkor v1 CPU may allocate TLB entries
>>>> using an incorrect ASID when TTBRx_EL1 is being updated. When the erratum
>>>> is triggered, page table entries using the new translation table base
>>>> address (BADDR) will be allocated into the TLB using the old ASID. All
>>>> circumstances leading to the incorrect ASID being cached in the TLB arise
>>>> when software writes TTBRx_EL1[ASID] and TTBRx_EL1[BADDR], a memory
>>>> operation is in the process of performing a translation using the specific
>>>> TTBRx_EL1 being written, and the memory operation uses a translation table
>>>> descriptor designated as non-global. EL2 and EL3 code changing the EL1&0
>>>> ASID is not subject to this erratum because hardware is prohibited from
>>>> performing translations from an out-of-context translation regime.
>>>>
>>>> Consider the following pseudo code.
>>>>
>>>>   write new BADDR and ASID values to TTBRx_EL1
>>>>
>>>> Replacing the above sequence with the one below will ensure that no TLB
>>>> entries with an incorrect ASID are used by software.
>>>>
>>>>   write reserved value to TTBRx_EL1[ASID]
>>>>   ISB
>>>>   write new value to TTBRx_EL1[BADDR]
>>>>   ISB
>>>>   write new value to TTBRx_EL1[ASID]
>>>>   ISB
>>>>
>>>> When the above sequence is used, page table entries using the new BADDR
>>>> value may still be incorrectly allocated into the TLB using the reserved
>>>> ASID. Yet this will not reduce functionality, since TLB entries incorrectly
>>>> tagged with the reserved ASID will never be hit by a later instruction.
>>>
>>> Based on my understanding that entries allocated to the reserved ASID
>>> will not be used for subsequent page table walks (and so we don't have
>>> asynchronous behaviour to contend with), this sounds fine to me.
>>>
>>> Thanks for taking the time to clarify the details on that.
>>>
>>>> Based on work by Shanker Donthineni <shankerd@codeaurora.org>
>>>>
>>>> Signed-off-by: Christopher Covington <cov@codeaurora.org>
>>>> ---
>>>>  Documentation/arm64/silicon-errata.txt |  1 +
>>>>  arch/arm64/Kconfig                     | 11 +++++++++++
>>>>  arch/arm64/include/asm/assembler.h     | 23 +++++++++++++++++++++++
>>>>  arch/arm64/include/asm/cpucaps.h       |  3 ++-
>>>>  arch/arm64/include/asm/mmu_context.h   |  8 +++++++-
>>>>  arch/arm64/kernel/cpu_errata.c         |  7 +++++++
>>>>  arch/arm64/mm/context.c                | 11 +++++++++++
>>>>  arch/arm64/mm/proc.S                   |  1 +
>>>>  8 files changed, 63 insertions(+), 2 deletions(-)
>>>
>>> Don't we need to use pre_ttbr0_update_workaround in <asm/asm-uaccess.h>
>>> for CONFIG_ARM64_SW_TTBR0_PAN? We implicitly switch to the reserved ASID
>>> for the empty table in __uaccess_ttbr0_disable.
>>>
>>> That also means we have to invalidate the reserved ASID so as to not
>>> accidentally hit while uaccess is disabled.
>>
>> The CPU in question (Falkor v1) has hardware PAN support. Do we need
>> to worry about including the workaround in the SW PAN code in that case?
> 
> Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> that case too, and hope that people do enable the HW version.

Okay, I'll do my best to add support for the SW PAN case. I rebased and
submitted v6 of the E1009 patch [1] so that it no longer depends on this
patch landing first, if you all are inclined to pick it up while work on
this E1003 patch continues.

1. https://patchwork.kernel.org/patch/9547923/

Thanks,
Christopher

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1571713

On Wed, Feb 01, 2017 at 11:29:22AM -0500, Christopher Covington wrote:
> On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> > Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> > enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> > that case too, and hope that people do enable the HW version.
> 
> Okay, I'll do my best to add support for the SW PAN case. I rebased and
> submitted v6 of the E1009 patch [1] so that it no longer depends on this
> patch landing first, if you all are inclined to pick it up while work on
> this E1003 patch continues.

The alternative is not enabling SW_PAN (at runtime) if this errata is
present, along with a warning stating that hardware-PAN should be
enabled in kconfig instead. Not sure what distributions will make of that
though.

> 1. https://patchwork.kernel.org/patch/9547923/

Yup, I queued that one locally and I'll push to -next tomorrow.

Thanks,

Will

[toc] | [prev] | [next] | [standalone]

#1571812

On Wed, Feb 01, 2017 at 04:33:58PM +0000, Will Deacon wrote:
> On Wed, Feb 01, 2017 at 11:29:22AM -0500, Christopher Covington wrote:
> > On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> > > Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> > > enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> > > that case too, and hope that people do enable the HW version.
> > 
> > Okay, I'll do my best to add support for the SW PAN case. I rebased and
> > submitted v6 of the E1009 patch [1] so that it no longer depends on this
> > patch landing first, if you all are inclined to pick it up while work on
> > this E1003 patch continues.
> 
> The alternative is not enabling SW_PAN (at runtime) if this errata is
> present, along with a warning stating that hardware-PAN should be
> enabled in kconfig instead. Not sure what distributions will make of that
> though.

The problem with this patch is that when ARM64_SW_TTBR0_PAN is enabled
and in the absence of hardware PAN (or ARM64_PAN disabled),
cpu_do_switch_mm is no longer called for user process switching, so the
workaround is pretty much useless.

I'm ok with adding the Kconfig dependency below to
QCOM_FALKOR_ERRATUM_1003:

	depends on !ARM64_SW_TTBR0_PAN || ARM64_PAN

together with a run-time warning if ARM64_SW_TTBR0_PAN is being used.

I'm not keen on adding the workaround to the uaccess macros. They are
complex enough already and people who do want SW PAN (and single Image)
would end up with 6-8 extra unnecessary nops on each side of a uaccess
(and nops are not entirely free).

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1571843

On Wed, Feb 01, 2017 at 05:36:09PM +0000, Catalin Marinas wrote:
> On Wed, Feb 01, 2017 at 04:33:58PM +0000, Will Deacon wrote:
> > On Wed, Feb 01, 2017 at 11:29:22AM -0500, Christopher Covington wrote:
> > > On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> > > > Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> > > > enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> > > > that case too, and hope that people do enable the HW version.
> > > 
> > > Okay, I'll do my best to add support for the SW PAN case. I rebased and
> > > submitted v6 of the E1009 patch [1] so that it no longer depends on this
> > > patch landing first, if you all are inclined to pick it up while work on
> > > this E1003 patch continues.
> > 
> > The alternative is not enabling SW_PAN (at runtime) if this errata is
> > present, along with a warning stating that hardware-PAN should be
> > enabled in kconfig instead. Not sure what distributions will make of that
> > though.
> 
> The problem with this patch is that when ARM64_SW_TTBR0_PAN is enabled
> and in the absence of hardware PAN (or ARM64_PAN disabled),
> cpu_do_switch_mm is no longer called for user process switching, so the
> workaround is pretty much useless.

Oh, I see what you mean now.

> I'm ok with adding the Kconfig dependency below to
> QCOM_FALKOR_ERRATUM_1003:
> 
> 	depends on !ARM64_SW_TTBR0_PAN || ARM64_PAN
> 
> together with a run-time warning if ARM64_SW_TTBR0_PAN is being used.

That makes it look like hardware-PAN is the cause of the erratum. Maybe
just select ARM64_PAN if the erratum workaround is selected, then
runtime warning if we find that the h/w doesn't have PAN but does have
the erratum (which should never fire)?

Will

[toc] | [prev] | [next] | [standalone]

#1571852

On Wed, Feb 01, 2017 at 05:49:34PM +0000, Catalin Marinas wrote:
> On Wed, Feb 01, 2017 at 05:41:05PM +0000, Will Deacon wrote:
> > On Wed, Feb 01, 2017 at 05:36:09PM +0000, Catalin Marinas wrote:
> > > On Wed, Feb 01, 2017 at 04:33:58PM +0000, Will Deacon wrote:
> > > > On Wed, Feb 01, 2017 at 11:29:22AM -0500, Christopher Covington wrote:
> > > > > On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> > > > > > Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> > > > > > enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> > > > > > that case too, and hope that people do enable the HW version.
> > > > > 
> > > > > Okay, I'll do my best to add support for the SW PAN case. I rebased and
> > > > > submitted v6 of the E1009 patch [1] so that it no longer depends on this
> > > > > patch landing first, if you all are inclined to pick it up while work on
> > > > > this E1003 patch continues.
> > > > 
> > > > The alternative is not enabling SW_PAN (at runtime) if this errata is
> > > > present, along with a warning stating that hardware-PAN should be
> > > > enabled in kconfig instead. Not sure what distributions will make of that
> > > > though.
> > > 
> > > The problem with this patch is that when ARM64_SW_TTBR0_PAN is enabled
> > > and in the absence of hardware PAN (or ARM64_PAN disabled),
> > > cpu_do_switch_mm is no longer called for user process switching, so the
> > > workaround is pretty much useless.
> > 
> > Oh, I see what you mean now.
> > 
> > > I'm ok with adding the Kconfig dependency below to
> > > QCOM_FALKOR_ERRATUM_1003:
> > > 
> > > 	depends on !ARM64_SW_TTBR0_PAN || ARM64_PAN
> > > 
> > > together with a run-time warning if ARM64_SW_TTBR0_PAN is being used.
> > 
> > That makes it look like hardware-PAN is the cause of the erratum.
> 
> With the right Kconfig comment we could make this clearer.

It's not just a comment though, the kconfig option for the workaround
will disappear from menuconfig as long as the dependencies aren't met.
The dependency is really that SW_PAN depends on !ERRATUM_1003, but that
doesn't work for the distributions.

> > Maybe
> > just select ARM64_PAN if the erratum workaround is selected, then
> > runtime warning if we find that the h/w doesn't have PAN but does have
> > the erratum (which should never fire)?
> 
> You still need this workaround even if you don't want any PAN (both sw
> and hw PAN disabled). I wouldn't want to select ARM64_PAN since it's not
> a dependency. It's more like if you do need a PAN, make sure you only
> use the hw one.

True, in the case that all PAN options are disabled we still want this
to work. How about:

  select ARM64_PAN if ARM64_SW_TTBR0_PAN

?

In fact, what's the reason for supporting SW_PAN and ARM64_PAN as a
config combination? Why not just have "PAN" that enables them both and
uses the hardware feature if it's there?

Will

[toc] | [prev] | [next] | [standalone]

#1571883

On Wed, Feb 01, 2017 at 05:59:48PM +0000, Will Deacon wrote:
> On Wed, Feb 01, 2017 at 05:49:34PM +0000, Catalin Marinas wrote:
> > On Wed, Feb 01, 2017 at 05:41:05PM +0000, Will Deacon wrote:
> > > On Wed, Feb 01, 2017 at 05:36:09PM +0000, Catalin Marinas wrote:
> > > > On Wed, Feb 01, 2017 at 04:33:58PM +0000, Will Deacon wrote:
> > > > > On Wed, Feb 01, 2017 at 11:29:22AM -0500, Christopher Covington wrote:
> > > > > > On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> > > > > > > Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> > > > > > > enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> > > > > > > that case too, and hope that people do enable the HW version.
> > > > > > 
> > > > > > Okay, I'll do my best to add support for the SW PAN case. I rebased and
> > > > > > submitted v6 of the E1009 patch [1] so that it no longer depends on this
> > > > > > patch landing first, if you all are inclined to pick it up while work on
> > > > > > this E1003 patch continues.
> > > > > 
> > > > > The alternative is not enabling SW_PAN (at runtime) if this errata is
> > > > > present, along with a warning stating that hardware-PAN should be
> > > > > enabled in kconfig instead. Not sure what distributions will make of that
> > > > > though.
> > > > 
> > > > The problem with this patch is that when ARM64_SW_TTBR0_PAN is enabled
> > > > and in the absence of hardware PAN (or ARM64_PAN disabled),
> > > > cpu_do_switch_mm is no longer called for user process switching, so the
> > > > workaround is pretty much useless.
> > > 
> > > Oh, I see what you mean now.
> > > 
> > > > I'm ok with adding the Kconfig dependency below to
> > > > QCOM_FALKOR_ERRATUM_1003:
> > > > 
> > > > 	depends on !ARM64_SW_TTBR0_PAN || ARM64_PAN
> > > > 
> > > > together with a run-time warning if ARM64_SW_TTBR0_PAN is being used.
> > > 
> > > That makes it look like hardware-PAN is the cause of the erratum.
> > 
> > With the right Kconfig comment we could make this clearer.
> 
> It's not just a comment though, the kconfig option for the workaround
> will disappear from menuconfig as long as the dependencies aren't met.
> The dependency is really that SW_PAN depends on !ERRATUM_1003, but that
> doesn't work for the distributions.

I agree.

> > > Maybe
> > > just select ARM64_PAN if the erratum workaround is selected, then
> > > runtime warning if we find that the h/w doesn't have PAN but does have
> > > the erratum (which should never fire)?
> > 
> > You still need this workaround even if you don't want any PAN (both sw
> > and hw PAN disabled). I wouldn't want to select ARM64_PAN since it's not
> > a dependency. It's more like if you do need a PAN, make sure you only
> > use the hw one.
> 
> True, in the case that all PAN options are disabled we still want this
> to work. How about:
> 
>   select ARM64_PAN if ARM64_SW_TTBR0_PAN

As I replied to myself, the above would work for me as well, so let's go
for this.

> In fact, what's the reason for supporting SW_PAN and ARM64_PAN as a
> config combination? Why not just have "PAN" that enables them both and
> uses the hardware feature if it's there?

Because SW PAN has a non-trivial performance hit. You would enable SW
PAN only if you are paranoid about security. HW PAN, OTOH, is very cheap
and I wouldn't want to miss enabling it in a single Image supporting
ARMv8.0 and ARMv8.1 just because SW PAN is slow on ARMv8.0.

IOW, ARM64_PAN is default y while ARM64_SW_TTBR0_PAN is default n.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1571884

On Wed, Feb 01, 2017 at 06:22:44PM +0000, Catalin Marinas wrote:
> On Wed, Feb 01, 2017 at 05:59:48PM +0000, Will Deacon wrote:
> > On Wed, Feb 01, 2017 at 05:49:34PM +0000, Catalin Marinas wrote:
> > > On Wed, Feb 01, 2017 at 05:41:05PM +0000, Will Deacon wrote:
> > > > Maybe
> > > > just select ARM64_PAN if the erratum workaround is selected, then
> > > > runtime warning if we find that the h/w doesn't have PAN but does have
> > > > the erratum (which should never fire)?
> > > 
> > > You still need this workaround even if you don't want any PAN (both sw
> > > and hw PAN disabled). I wouldn't want to select ARM64_PAN since it's not
> > > a dependency. It's more like if you do need a PAN, make sure you only
> > > use the hw one.
> > 
> > True, in the case that all PAN options are disabled we still want this
> > to work. How about:
> > 
> >   select ARM64_PAN if ARM64_SW_TTBR0_PAN
> 
> As I replied to myself, the above would work for me as well, so let's go
> for this.
> 
> > In fact, what's the reason for supporting SW_PAN and ARM64_PAN as a
> > config combination? Why not just have "PAN" that enables them both and
> > uses the hardware feature if it's there?
> 
> Because SW PAN has a non-trivial performance hit. You would enable SW
> PAN only if you are paranoid about security. HW PAN, OTOH, is very cheap
> and I wouldn't want to miss enabling it in a single Image supporting
> ARMv8.0 and ARMv8.1 just because SW PAN is slow on ARMv8.0.
> 
> IOW, ARM64_PAN is default y while ARM64_SW_TTBR0_PAN is default n.

Ok, in that case, then how about another permutation: we make
ARM64_SW_TTBR0_PAN depend on ARM64_PAN? Then when you select "PAN Support"
you get a new menu option underneath it for the emulation? I think that
solves the erratum case and the use-case above.

Will

[toc] | [prev] | [next] | [standalone]

#1571889

On Wed, Feb 01, 2017 at 06:34:01PM +0000, Will Deacon wrote:
> On Wed, Feb 01, 2017 at 06:22:44PM +0000, Catalin Marinas wrote:
> > On Wed, Feb 01, 2017 at 05:59:48PM +0000, Will Deacon wrote:
> > > On Wed, Feb 01, 2017 at 05:49:34PM +0000, Catalin Marinas wrote:
> > > > On Wed, Feb 01, 2017 at 05:41:05PM +0000, Will Deacon wrote:
> > > > > Maybe
> > > > > just select ARM64_PAN if the erratum workaround is selected, then
> > > > > runtime warning if we find that the h/w doesn't have PAN but does have
> > > > > the erratum (which should never fire)?
> > > > 
> > > > You still need this workaround even if you don't want any PAN (both sw
> > > > and hw PAN disabled). I wouldn't want to select ARM64_PAN since it's not
> > > > a dependency. It's more like if you do need a PAN, make sure you only
> > > > use the hw one.
> > > 
> > > True, in the case that all PAN options are disabled we still want this
> > > to work. How about:
> > > 
> > >   select ARM64_PAN if ARM64_SW_TTBR0_PAN
> > 
> > As I replied to myself, the above would work for me as well, so let's go
> > for this.
> > 
> > > In fact, what's the reason for supporting SW_PAN and ARM64_PAN as a
> > > config combination? Why not just have "PAN" that enables them both and
> > > uses the hardware feature if it's there?
> > 
> > Because SW PAN has a non-trivial performance hit. You would enable SW
> > PAN only if you are paranoid about security. HW PAN, OTOH, is very cheap
> > and I wouldn't want to miss enabling it in a single Image supporting
> > ARMv8.0 and ARMv8.1 just because SW PAN is slow on ARMv8.0.
> > 
> > IOW, ARM64_PAN is default y while ARM64_SW_TTBR0_PAN is default n.
> 
> Ok, in that case, then how about another permutation: we make
> ARM64_SW_TTBR0_PAN depend on ARM64_PAN? Then when you select "PAN Support"
> you get a new menu option underneath it for the emulation? I think that
> solves the erratum case and the use-case above.

The problem is that ARM64_PAN is an ARMv8.1 feature and currently
grouped accordingly in Kconfig. ARM64_SW_TTBR0_PAN is complementary (and
even not recommended on ARMv8.1). We can do this if we break the ARMv8.x
feature grouping but just for this erratum, I don't think it's worth.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1576191

On 02/01/2017 01:38 PM, Catalin Marinas wrote:
> On Wed, Feb 01, 2017 at 06:34:01PM +0000, Will Deacon wrote:
>> On Wed, Feb 01, 2017 at 06:22:44PM +0000, Catalin Marinas wrote:
>>> On Wed, Feb 01, 2017 at 05:59:48PM +0000, Will Deacon wrote:
>>>> On Wed, Feb 01, 2017 at 05:49:34PM +0000, Catalin Marinas wrote:
>>>>> On Wed, Feb 01, 2017 at 05:41:05PM +0000, Will Deacon wrote:
>>>>>> Maybe
>>>>>> just select ARM64_PAN if the erratum workaround is selected, then
>>>>>> runtime warning if we find that the h/w doesn't have PAN but does have
>>>>>> the erratum (which should never fire)?
>>>>>
>>>>> You still need this workaround even if you don't want any PAN (both sw
>>>>> and hw PAN disabled). I wouldn't want to select ARM64_PAN since it's not
>>>>> a dependency. It's more like if you do need a PAN, make sure you only
>>>>> use the hw one.
>>>>
>>>> True, in the case that all PAN options are disabled we still want this
>>>> to work. How about:
>>>>
>>>>   select ARM64_PAN if ARM64_SW_TTBR0_PAN
>>>
>>> As I replied to myself, the above would work for me as well, so let's go
>>> for this.
>>>
>>>> In fact, what's the reason for supporting SW_PAN and ARM64_PAN as a
>>>> config combination? Why not just have "PAN" that enables them both and
>>>> uses the hardware feature if it's there?
>>>
>>> Because SW PAN has a non-trivial performance hit. You would enable SW
>>> PAN only if you are paranoid about security. HW PAN, OTOH, is very cheap
>>> and I wouldn't want to miss enabling it in a single Image supporting
>>> ARMv8.0 and ARMv8.1 just because SW PAN is slow on ARMv8.0.
>>>
>>> IOW, ARM64_PAN is default y while ARM64_SW_TTBR0_PAN is default n.
>>
>> Ok, in that case, then how about another permutation: we make
>> ARM64_SW_TTBR0_PAN depend on ARM64_PAN? Then when you select "PAN Support"
>> you get a new menu option underneath it for the emulation? I think that
>> solves the erratum case and the use-case above.
> 
> The problem is that ARM64_PAN is an ARMv8.1 feature and currently
> grouped accordingly in Kconfig. ARM64_SW_TTBR0_PAN is complementary (and
> even not recommended on ARMv8.1). We can do this if we break the ARMv8.x
> feature grouping but just for this erratum, I don't think it's worth.

Thanks all. I've used "select ARM64_PAN if ARM64_SW_TTBR0_PAN" in v6.

Thanks,
Cov

-- 
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm
Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code
Aurora Forum, a Linux Foundation Collaborative Project.

[toc] | [prev] | [next] | [standalone]

#1571859

On Wed, Feb 01, 2017 at 05:49:34PM +0000, Catalin Marinas wrote:
> On Wed, Feb 01, 2017 at 05:41:05PM +0000, Will Deacon wrote:
> > On Wed, Feb 01, 2017 at 05:36:09PM +0000, Catalin Marinas wrote:
> > > On Wed, Feb 01, 2017 at 04:33:58PM +0000, Will Deacon wrote:
> > > > On Wed, Feb 01, 2017 at 11:29:22AM -0500, Christopher Covington wrote:
> > > > > On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> > > > > > Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> > > > > > enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> > > > > > that case too, and hope that people do enable the HW version.
> > > > > 
> > > > > Okay, I'll do my best to add support for the SW PAN case. I rebased and
> > > > > submitted v6 of the E1009 patch [1] so that it no longer depends on this
> > > > > patch landing first, if you all are inclined to pick it up while work on
> > > > > this E1003 patch continues.
> > > > 
> > > > The alternative is not enabling SW_PAN (at runtime) if this errata is
> > > > present, along with a warning stating that hardware-PAN should be
> > > > enabled in kconfig instead. Not sure what distributions will make of that
> > > > though.
> > > 
> > > The problem with this patch is that when ARM64_SW_TTBR0_PAN is enabled
> > > and in the absence of hardware PAN (or ARM64_PAN disabled),
> > > cpu_do_switch_mm is no longer called for user process switching, so the
> > > workaround is pretty much useless.
> > 
> > Oh, I see what you mean now.
> > 
> > > I'm ok with adding the Kconfig dependency below to
> > > QCOM_FALKOR_ERRATUM_1003:
> > > 
> > > 	depends on !ARM64_SW_TTBR0_PAN || ARM64_PAN
> > > 
> > > together with a run-time warning if ARM64_SW_TTBR0_PAN is being used.
> > 
> > That makes it look like hardware-PAN is the cause of the erratum.
> 
> With the right Kconfig comment we could make this clearer.
> 
> > Maybe
> > just select ARM64_PAN if the erratum workaround is selected, then
> > runtime warning if we find that the h/w doesn't have PAN but does have
> > the erratum (which should never fire)?
> 
> You still need this workaround even if you don't want any PAN (both sw
> and hw PAN disabled). I wouldn't want to select ARM64_PAN since it's not
> a dependency. It's more like if you do need a PAN, make sure you only
> use the hw one.

Alternatively, your select idea could be refined to:

	select ARM64_PAN if ARM64_SW_TTBR0_PAN

but we still need a proper comment as people would wonder what this is
for.

-- 
Catalin

[toc] | [prev] | [next] | [standalone]

#1571863

On Wed, Feb 01, 2017 at 05:41:05PM +0000, Will Deacon wrote:
> On Wed, Feb 01, 2017 at 05:36:09PM +0000, Catalin Marinas wrote:
> > On Wed, Feb 01, 2017 at 04:33:58PM +0000, Will Deacon wrote:
> > > On Wed, Feb 01, 2017 at 11:29:22AM -0500, Christopher Covington wrote:
> > > > On 01/31/2017 12:56 PM, Marc Zyngier wrote:
> > > > > Given that all ARMv8 CPUs can support SW_PAN, it is more likely to be
> > > > > enabled than the ARMv8.1 PAN. I'd vote for supporting the workaround in
> > > > > that case too, and hope that people do enable the HW version.
> > > > 
> > > > Okay, I'll do my best to add support for the SW PAN case. I rebased and
> > > > submitted v6 of the E1009 patch [1] so that it no longer depends on this
> > > > patch landing first, if you all are inclined to pick it up while work on
> > > > this E1003 patch continues.
> > > 
> > > The alternative is not enabling SW_PAN (at runtime) if this errata is
> > > present, along with a warning stating that hardware-PAN should be
> > > enabled in kconfig instead. Not sure what distributions will make of that
> > > though.
> > 
> > The problem with this patch is that when ARM64_SW_TTBR0_PAN is enabled
> > and in the absence of hardware PAN (or ARM64_PAN disabled),
> > cpu_do_switch_mm is no longer called for user process switching, so the
> > workaround is pretty much useless.
> 
> Oh, I see what you mean now.
> 
> > I'm ok with adding the Kconfig dependency below to
> > QCOM_FALKOR_ERRATUM_1003:
> > 
> > 	depends on !ARM64_SW_TTBR0_PAN || ARM64_PAN
> > 
> > together with a run-time warning if ARM64_SW_TTBR0_PAN is being used.
> 
> That makes it look like hardware-PAN is the cause of the erratum.

With the right Kconfig comment we could make this clearer.

> Maybe
> just select ARM64_PAN if the erratum workaround is selected, then
> runtime warning if we find that the h/w doesn't have PAN but does have
> the erratum (which should never fire)?

You still need this workaround even if you don't want any PAN (both sw
and hw PAN disabled). I wouldn't want to select ARM64_PAN since it's not
a dependency. It's more like if you do need a PAN, make sure you only
use the hw one.

-- 
Catalin

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web