Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752431AbdC2PMO (ORCPT ); Wed, 29 Mar 2017 11:12:14 -0400 Received: from mail-lf0-f52.google.com ([209.85.215.52]:33296 "EHLO mail-lf0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751446AbdC2PMM (ORCPT ); Wed, 29 Mar 2017 11:12:12 -0400 Date: Wed, 29 Mar 2017 17:12:05 +0200 From: Daniel Lezcano To: Marc Zyngier Cc: linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, Mark Rutland , Catalin Marinas , Will Deacon , Scott Wood , Hanjun Guo , Ding Tianhong , dann frazier Subject: Re: [PATCH v2 06/18] arm64: arch_timer: Add infrastructure for multiple erratum detection methods Message-ID: <20170329151205.GM2123@mai> References: <20170327075628.GE24630@mai> <20170328133438.GB2123@mai> <20170328143633.GC2123@mai> <20170328145524.GD2123@mai> <645ab4be-2730-3197-9d70-4e44692ea693@arm.com> <20170329142723.GK2123@mai> <36733343-8f16-f009-dd4d-1ee63614b053@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <36733343-8f16-f009-dd4d-1ee63614b053@arm.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4624 Lines: 102 On Wed, Mar 29, 2017 at 03:56:52PM +0100, Marc Zyngier wrote: > 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. No. I had a comment regarding global/local but it is apparently not possible. Let put the concept apart and move forward. Thanks. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog