All of lore.kernel.org
 help / color / mirror / Atom feed
From: Marc Zyngier <marc.zyngier@arm.com>
To: Daniel Lezcano <daniel.lezcano@linaro.org>
Cc: linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org, Mark Rutland <mark.rutland@arm.com>,
	Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>, Scott Wood <oss@buserror.net>,
	Hanjun Guo <hanjun.guo@linaro.org>,
	Ding Tianhong <dingtianhong@huawei.com>,
	dann frazier <dann.frazier@canonical.com>
Subject: Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods
Date: Wed, 29 Mar 2017 15:56:52 +0100	[thread overview]
Message-ID: <36733343-8f16-f009-dd4d-1ee63614b053@arm.com> (raw)
In-Reply-To: <20170329142723.GK2123@mai>

On 29/03/17 15:27, Daniel Lezcano wrote:
> On Tue, Mar 28, 2017 at 04:38:41PM +0100, Marc Zyngier wrote:
>> On 28/03/17 15:55, Daniel Lezcano wrote:
>>> On Tue, Mar 28, 2017 at 03:48:23PM +0100, Marc Zyngier wrote:
>>>> On 28/03/17 15:36, Daniel Lezcano wrote:
>>>>> On Tue, Mar 28, 2017 at 03:07:52PM +0100, Marc Zyngier wrote:
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>>>> -bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>>>>>>>>> -					 const void *arg)
>>>>>>>>> +bool arch_timer_check_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>>>>>>>>> +				  const void *arg)
>>>>>>>>>  {
>>>>>>>>> -	return cpus_have_cap((uintptr_t)wa->id);
>>>>>>>>> +	return cpus_have_cap((uintptr_t)wa->id) | this_cpu_has_cap((uintptr_t)wa->id);
>>>>>>>>
>>>>>>>> Not quite. Here, you're making all capability-based errata to be be
>>>>>>>> global (if a single CPU in the system has a capability, then by
>>>>>>>> transitivity cpus_have_cap returns true). If that's a big-little system,
>>>>>>>> you end-up applying the workaround to all CPUs, including those unaffected.
>>>>>>>>
>>>>>>>> I'd rather drop cpus_have_cap altogether and rely on individual CPU
>>>>>>>> matching (since we don't have a need for a global capability erratum
>>>>>>>> handling yet).
>>>>>>>
>>>>>>> Ok, thanks.
>>>>>>
>>>>>> Quick update. I've just implemented this, and found out that getting rid
>>>>>> of local/global has an unfortunate effect:
>>>>>>
>>>>>> Since we only probe the global errata (using ACPI for example) on the
>>>>>> boot CPU path, we lose propagation of the erratum across the secondary
>>>>>> CPUs. One way of solving this is to convert the secondary boot path to
>>>>>> be aware of DT vs ACPI vs detection method of the month. Which isn't
>>>>>> easy, since by the time we boot secondary CPUs, we don't have the
>>>>>> pointers to the various ACPI tables anymore. Also, assuming we were
>>>>>> careful and saved the pointers, the tables may have been unmapped. Fun.
>>>>>
>>>>> My proposal was supposed to prevent that. The detecion is done in the
>>>>> subsystems, ACPI detects ACPI errata, DT detects DT errata and CPU detects CPU
>>>>> errata. The drivers get the errata and enable the workaround. The id
>>>>> association <-> errata self contains errata types (void *, char *, int). So
>>>>> everything can be done in a CPU basis without local / global dance.
>>>>
>>>> I'm sorry, but it feels like a Jumbo-Jet sized hammer to try and squash
>>>> a fly (I'm staying away from the frozen shark metaphor here). You're
>>>> willing to add a whole list of things with private ids that need
>>>> matching to kill a flag? I don't think this buys us anything but extra
>>>> complexity and another maintenance headache.
>>>
>>> Well, it is like your approach except it is split in two steps.
>>>
>>> Can you explain where is the extra complexity ? May be I am missing the point.
>>
>> This is how I understand your approach:
>>
>> - Boot the first CPU
>> - Build a list of errata discovered at that time
>> - Apply erratum on the boot CPU if required, using a yet-to-be-invented
>> private id matching mechanism,
>> - Boot a secondary CPU
>> - Apply erratum if required, parsing the list
>> - Realise that you don't have the full list (this CPU comes with an
>> erratum that was not in the initial list)
>> - Add more to the list
>> - Apply erratum, using the same matching mechanism
>>
>> This is mine:
>>
>> - Boot the first CPU
>> - Apply global erratum to all CPUs
>> - Apply local erratum
>> - Boot a secondary CPU
>> - Apply local erratum
>>
>> In my case, everything is static, and I don't need to rematch each CPU
>> against the list of globally applicable errata.
>>
>> If my understanding is flawed, let me know.
> 
> Any of our understanding is flawed. I think that needs a maturation period.

Well, these patches have been maturing for a while, and time is running
out. If you have a better idea that is more than a concept, please post
the code, I'd be happy to review it.

Thanks,

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

WARNING: multiple messages have this Message-ID (diff)
From: marc.zyngier@arm.com (Marc Zyngier)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods
Date: Wed, 29 Mar 2017 15:56:52 +0100	[thread overview]
Message-ID: <36733343-8f16-f009-dd4d-1ee63614b053@arm.com> (raw)
In-Reply-To: <20170329142723.GK2123@mai>

On 29/03/17 15:27, Daniel Lezcano wrote:
> On Tue, Mar 28, 2017 at 04:38:41PM +0100, Marc Zyngier wrote:
>> On 28/03/17 15:55, Daniel Lezcano wrote:
>>> On Tue, Mar 28, 2017 at 03:48:23PM +0100, Marc Zyngier wrote:
>>>> On 28/03/17 15:36, Daniel Lezcano wrote:
>>>>> On Tue, Mar 28, 2017 at 03:07:52PM +0100, Marc Zyngier wrote:
>>>>>
>>>>> [ ... ]
>>>>>
>>>>>>>>> -bool arch_timer_check_global_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>>>>>>>>> -					 const void *arg)
>>>>>>>>> +bool arch_timer_check_cap_erratum(const struct arch_timer_erratum_workaround *wa,
>>>>>>>>> +				  const void *arg)
>>>>>>>>>  {
>>>>>>>>> -	return cpus_have_cap((uintptr_t)wa->id);
>>>>>>>>> +	return cpus_have_cap((uintptr_t)wa->id) | this_cpu_has_cap((uintptr_t)wa->id);
>>>>>>>>
>>>>>>>> Not quite. Here, you're making all capability-based errata to be be
>>>>>>>> global (if a single CPU in the system has a capability, then by
>>>>>>>> transitivity cpus_have_cap returns true). If that's a big-little system,
>>>>>>>> you end-up applying the workaround to all CPUs, including those unaffected.
>>>>>>>>
>>>>>>>> I'd rather drop cpus_have_cap altogether and rely on individual CPU
>>>>>>>> matching (since we don't have a need for a global capability erratum
>>>>>>>> handling yet).
>>>>>>>
>>>>>>> Ok, thanks.
>>>>>>
>>>>>> Quick update. I've just implemented this, and found out that getting rid
>>>>>> of local/global has an unfortunate effect:
>>>>>>
>>>>>> Since we only probe the global errata (using ACPI for example) on the
>>>>>> boot CPU path, we lose propagation of the erratum across the secondary
>>>>>> CPUs. One way of solving this is to convert the secondary boot path to
>>>>>> be aware of DT vs ACPI vs detection method of the month. Which isn't
>>>>>> easy, since by the time we boot secondary CPUs, we don't have the
>>>>>> pointers to the various ACPI tables anymore. Also, assuming we were
>>>>>> careful and saved the pointers, the tables may have been unmapped. Fun.
>>>>>
>>>>> My proposal was supposed to prevent that. The detecion is done in the
>>>>> subsystems, ACPI detects ACPI errata, DT detects DT errata and CPU detects CPU
>>>>> errata. The drivers get the errata and enable the workaround. The id
>>>>> association <-> errata self contains errata types (void *, char *, int). So
>>>>> everything can be done in a CPU basis without local / global dance.
>>>>
>>>> I'm sorry, but it feels like a Jumbo-Jet sized hammer to try and squash
>>>> a fly (I'm staying away from the frozen shark metaphor here). You're
>>>> willing to add a whole list of things with private ids that need
>>>> matching to kill a flag? I don't think this buys us anything but extra
>>>> complexity and another maintenance headache.
>>>
>>> Well, it is like your approach except it is split in two steps.
>>>
>>> Can you explain where is the extra complexity ? May be I am missing the point.
>>
>> This is how I understand your approach:
>>
>> - Boot the first CPU
>> - Build a list of errata discovered at that time
>> - Apply erratum on the boot CPU if required, using a yet-to-be-invented
>> private id matching mechanism,
>> - Boot a secondary CPU
>> - Apply erratum if required, parsing the list
>> - Realise that you don't have the full list (this CPU comes with an
>> erratum that was not in the initial list)
>> - Add more to the list
>> - Apply erratum, using the same matching mechanism
>>
>> This is mine:
>>
>> - Boot the first CPU
>> - Apply global erratum to all CPUs
>> - Apply local erratum
>> - Boot a secondary CPU
>> - Apply local erratum
>>
>> In my case, everything is static, and I don't need to rematch each CPU
>> against the list of globally applicable errata.
>>
>> If my understanding is flawed, let me know.
> 
> Any of our understanding is flawed. I think that needs a maturation period.

Well, these patches have been maturing for a while, and time is running
out. If you have a better idea that is more than a concept, please post
the code, I'd be happy to review it.

Thanks,

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

  reply	other threads:[~2017-03-29 14:57 UTC|newest]

Thread overview: 88+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-03-20 17:48 [PATCH v2 00/18] clocksource/arch_timer: Errata workaround infrastructure rework Marc Zyngier
2017-03-20 17:48 ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 01/18] arm64: Allow checking of a CPU-local erratum Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-22  9:22   ` Daniel Lezcano
2017-03-22  9:22     ` Daniel Lezcano
2017-03-20 17:48 ` [PATCH v2 02/18] arm64: Add CNTVCT_EL0 trap handler Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 03/18] arm64: Define Cortex-A73 MIDR Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 04/18] arm64: cpu_errata: Allow an erratum to be match for all revisions of a core Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-22 14:57   ` Daniel Lezcano
2017-03-22 14:57     ` Daniel Lezcano
2017-03-20 17:48 ` [PATCH v2 05/18] arm64: cpu_errata: Add capability to advertise Cortex-A73 erratum 858921 Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-22 15:01   ` Daniel Lezcano
2017-03-22 15:01     ` Daniel Lezcano
2017-03-20 17:48 ` [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-22 15:41   ` Daniel Lezcano
2017-03-22 15:41     ` Daniel Lezcano
2017-03-22 15:53     ` Marc Zyngier
2017-03-22 15:53       ` Marc Zyngier
2017-03-22 15:59     ` Marc Zyngier
2017-03-22 15:59       ` Marc Zyngier
2017-03-22 16:52       ` Daniel Lezcano
2017-03-22 16:52         ` Daniel Lezcano
2017-03-23 17:30       ` Daniel Lezcano
2017-03-23 17:30         ` Daniel Lezcano
2017-03-24 13:51         ` Marc Zyngier
2017-03-24 13:51           ` Marc Zyngier
2017-03-27  7:56           ` Daniel Lezcano
2017-03-27  7:56             ` Daniel Lezcano
2017-03-28 13:07             ` Marc Zyngier
2017-03-28 13:07               ` Marc Zyngier
2017-03-28 13:34               ` Daniel Lezcano
2017-03-28 13:34                 ` Daniel Lezcano
2017-03-28 14:07                 ` Marc Zyngier
2017-03-28 14:07                   ` Marc Zyngier
2017-03-28 14:36                   ` Daniel Lezcano
2017-03-28 14:36                     ` Daniel Lezcano
2017-03-28 14:48                     ` Marc Zyngier
2017-03-28 14:48                       ` Marc Zyngier
2017-03-28 14:55                       ` Daniel Lezcano
2017-03-28 14:55                         ` Daniel Lezcano
2017-03-28 15:38                         ` Marc Zyngier
2017-03-28 15:38                           ` Marc Zyngier
2017-03-29 14:27                           ` Daniel Lezcano
2017-03-29 14:27                             ` Daniel Lezcano
2017-03-29 14:56                             ` Marc Zyngier [this message]
2017-03-29 14:56                               ` Marc Zyngier
2017-03-29 15:12                               ` Daniel Lezcano
2017-03-29 15:12                                 ` Daniel Lezcano
2017-03-24 17:48   ` dann frazier
2017-03-24 17:48     ` dann frazier
2017-03-24 18:00     ` Marc Zyngier
2017-03-24 18:00       ` Marc Zyngier
2017-03-30  9:28       ` Daniel Lezcano
2017-03-30  9:28         ` Daniel Lezcano
2017-03-20 17:48 ` [PATCH v2 07/18] arm64: arch_timer: Add erratum handler for globally defined capability Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 08/18] arm64: arch_timer: Add erratum handler for CPU-specific capability Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 09/18] arm64: arch_timer: Move arch_timer_reg_read/write around Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-22 15:47   ` Daniel Lezcano
2017-03-22 15:47     ` Daniel Lezcano
2017-03-20 17:48 ` [PATCH v2 10/18] arm64: arch_timer: Get rid of erratum_workaround_set_sne Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 11/18] arm64: arch_timer: Rework the set_next_event workarounds Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 12/18] arm64: arch_timer: Make workaround methods optional Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 13/18] arm64: arch_timer: Allows a CPU-specific erratum to only affect a subset of CPUs Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 14/18] arm64: arch_timer: Move clocksource_counter and co around Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 15/18] arm64: arch_timer: Enable CNTVCT_EL0 trap if workaround is enabled Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 16/18] arm64: arch_timer: Workaround for Cortex-A73 erratum 858921 Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 17/18] arm64: arch_timer: Allow erratum matching with ACPI OEM information Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-20 17:48 ` [PATCH v2 18/18] arm64: arch_timer: Add HISILICON_ERRATUM_161010101 ACPI matching data Marc Zyngier
2017-03-20 17:48   ` Marc Zyngier
2017-03-22 13:56 ` [PATCH v2 00/18] clocksource/arch_timer: Errata workaround infrastructure rework Ding Tianhong
2017-03-22 13:56   ` Ding Tianhong

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=36733343-8f16-f009-dd4d-1ee63614b053@arm.com \
    --to=marc.zyngier@arm.com \
    --cc=catalin.marinas@arm.com \
    --cc=daniel.lezcano@linaro.org \
    --cc=dann.frazier@canonical.com \
    --cc=dingtianhong@huawei.com \
    --cc=hanjun.guo@linaro.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mark.rutland@arm.com \
    --cc=oss@buserror.net \
    --cc=will.deacon@arm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.