This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCH v3 16/28] arm64/sve: Probe SVE capabilities and usable vector lengths
- From: Dave Martin <Dave dot Martin at arm dot com>
- To: Suzuki K Poulose <Suzuki dot Poulose at arm dot com>
- Cc: linux-arch at vger dot kernel dot org, Okamoto Takayuki <tokamoto at jp dot fujitsu dot com>, libc-alpha at sourceware dot org, Ard Biesheuvel <ard dot biesheuvel at linaro dot org>, Szabolcs Nagy <szabolcs dot nagy at arm dot com>, Catalin Marinas <catalin dot marinas at arm dot com>, Will Deacon <will dot deacon at arm dot com>, Richard Sandiford <richard dot sandiford at arm dot com>, Alex Bennée <alex dot bennee at linaro dot org>, kvmarm at lists dot cs dot columbia dot edu, linux-arm-kernel at lists dot infradead dot org
- Date: Mon, 16 Oct 2017 17:55:53 +0100
- Subject: Re: [PATCH v3 16/28] arm64/sve: Probe SVE capabilities and usable vector lengths
- Authentication-results: sourceware.org; auth=none
- References: <1507660725-7986-1-git-send-email-Dave.Martin@arm.com> <1507660725-7986-17-git-send-email-Dave.Martin@arm.com> <a39354fb-9f21-47ff-2ad9-b5bcb04e9990@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>
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 <suzuki.poulose@arm.com>
> >>>
> >>>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).
With the above, are you happy for me to apply your Reviewed-by, or would
you prefer to wait for the respin?
Cheers
---Dave