On 16/10/17 17:55, Dave Martin wrote:

On Mon, Oct 16, 2017 at 05:47:16PM +0100, Suzuki K Poulose wrote:

quoted

On 16/10/17 17:44, Dave Martin wrote:

quoted

On Mon, Oct 16, 2017 at 05:27:59PM +0100, Suzuki K Poulose wrote:

quoted

On 16/10/17 16:46, Dave Martin wrote:

quoted

On Thu, Oct 12, 2017 at 01:56:51PM +0100, Suzuki K Poulose wrote:

quoted

On 10/10/17 19:38, Dave Martin wrote:

[...]

quoted

quoted

quoted

quoted

@@ -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.

quoted

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 [off-list ref]