From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 53229 invoked by alias); 16 Oct 2017 16:58:52 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Archive: List-Post: List-Help: , Sender: libc-alpha-owner@sourceware.org Received: (qmail 53215 invoked by uid 89); 16 Oct 2017 16:58:52 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=catching X-HELO: foss.arm.com Subject: Re: [PATCH v3 16/28] arm64/sve: Probe SVE capabilities and usable vector lengths To: Dave Martin Cc: linux-arch@vger.kernel.org, Okamoto Takayuki , libc-alpha@sourceware.org, Ard Biesheuvel , Szabolcs Nagy , Catalin Marinas , Will Deacon , Richard Sandiford , =?UTF-8?Q?Alex_Benn=c3=a9e?= , kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org References: <1507660725-7986-1-git-send-email-Dave.Martin@arm.com> <1507660725-7986-17-git-send-email-Dave.Martin@arm.com> <20171016154557.GR19485@e103592.cambridge.arm.com> <812b6d11-2458-6d5d-c490-3421f7d3afb3@arm.com> <20171016164407.GS19485@e103592.cambridge.arm.com> <7854a2cc-d3b1-ac76-8d12-2aa5e1be7aca@arm.com> <20171016165553.GT19485@e103592.cambridge.arm.com> From: Suzuki K Poulose Message-ID: Date: Mon, 16 Oct 2017 16:58:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.3.0 MIME-Version: 1.0 In-Reply-To: <20171016165553.GT19485@e103592.cambridge.arm.com> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit X-SW-Source: 2017-10/txt/msg00702.txt.bz2 On 16/10/17 17:55, Dave Martin wrote: > On Mon, Oct 16, 2017 at 05:47:16PM +0100, Suzuki K Poulose wrote: >> On 16/10/17 17:44, Dave Martin wrote: >>> On Mon, Oct 16, 2017 at 05:27:59PM +0100, Suzuki K Poulose wrote: >>>> On 16/10/17 16:46, Dave Martin wrote: >>>>> On Thu, Oct 12, 2017 at 01:56:51PM +0100, Suzuki K Poulose wrote: >>>>>> On 10/10/17 19:38, Dave Martin wrote: >>> >>> [...] >>> >>>>>>> @@ -670,6 +689,14 @@ void update_cpu_features(int cpu, >>>>>>> info->reg_mvfr2, boot->reg_mvfr2); >>>>>>> } >>>>>>> + if (id_aa64pfr0_sve(info->reg_id_aa64pfr0)) { >>>>>>> + taint |= check_update_ftr_reg(SYS_ZCR_EL1, cpu, >>>>>>> + info->reg_zcr, boot->reg_zcr); >>>>>>> + >>>>>>> + if (!sys_caps_initialised) >>>>>>> + sve_update_vq_map(); >>>>>>> + } >>>>>> >>>>>> nit: I am not sure if we should also check if the "current" sanitised value >>>>>> of the id_aa64pfr0 also supports sve and skip the update if it isn't. The code >>>>>> is as such fine without the check, its just that we can avoid computing the >>>>>> map. It is in the CPU boot up path and hence is not performance critical. >>>>>> So, either way we are fine. >>>>>> >>>>>> Reviewed-by: Suzuki K Poulose >>>>> >>>>> I think I prefer to avoid adding extra code to optimise the "broken SoC >>>>> design" case. >>>>> >>>> >>>> Sure. >>>> >>>>> Maybe we could revisit this later if needed. >>>>> >>>>> Can you suggest some code? Maybe the check is simpler than I think. >>>> >>>> Something like : >>>> >>>> if (id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_IDAA64PFR0)) && >>>> id_aa64pfr0_sve(id_aa64pfr0)) { >>>> ... >>>> } >>>> >>>> should be enough. >>>> >>>> Or even we could hack it to : >>>> >>>> if (id_aa64pfr0_sve(id_aa64pfr0 | read_sanitised_ftr_reg(SYS_IDAA64PFR0))) >>>> >>>> As I mentioned, the code as such is fine. Its just that we try to detect >>>> if the SVE is already moot and skip the steps for this CPU. >>> >>> How about the following, keeping the outer >>> if(id_aa64pfr0_sve(int->reg_id_aa64pfr0)) from my current code: >>> >>> - if (!sys_caps_initialised) >>> + /* Probe vector lengths, unless we already gave up on SVE */ >>> + if (id_aa64pfr0_sve(read_sanitised_ftr_reg(ID_AA64PFR0_SVE)) && >>> + !sys_caps_initialised) >>> sve_update_vq_map(); >> >> Yep, that looks neater. > > Sorry, that should have been > > if (id_aa64pfr0_sve(read_sanitised_ftr_reg(SYS_ID_AA64PFR0_EL1)) && > > (Disturbingly, the original does build and then hits a BUG(), because > ID_AA64PFR0_SVE happens to be defined). Ouch ! I didn't notice that ;-). Good to see the BUG() catching such mistakes. > > > With the above, are you happy for me to apply your Reviewed-by, or would > you prefer to wait for the respin? With the changes as we discussed above, please feel free to add : Reviewed-by : Suzuki K Poulose