linux-kernel - Re: [PATCH v3 09/24] arm64: Keep track of CPU feature registers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 15 Oct 2015 11:36:12 +0100
From:	Catalin Marinas <catalin.marinas@....com>
To:	"Suzuki K. Poulose" <suzuki.poulose@....com>
Cc:	linux-arm-kernel@...ts.infradead.org, mark.rutland@....com,
	Vladimir.Murzin@....com, steve.capper@...aro.org,
	ryan.arnold@...aro.org, ard.biesheuvel@...aro.org,
	marc.zyngier@....com, will.deacon@....com,
	linux-kernel@...r.kernel.org, edward.nevill@...aro.org,
	aph@...hat.com, james.morse@....com, andre.przywara@....com,
	dave.martin@....com, christoffer.dall@...aro.org
Subject: Re: [PATCH v3 09/24] arm64: Keep track of CPU feature registers

Hi Suzuki,

Some minor comments below.

On Tue, Oct 13, 2015 at 06:22:17PM +0100, Suzuki K. Poulose wrote:
> This patch adds an infrastructure to keep track of the CPU feature
> registers on the system. For each register, the infrastructure keeps
> track of the system wide safe value of the feature bits. Also, tracks
> the which fields of a register should be matched strictly across all
> the CPUs on the system for the SANITY check infrastructure.
> 
> The feature bits are classified as one of SCALAR_MIN, SCALAR_MAX and DISCRETE
> depending on the implication of the possible values. This information
> is used to decide the safe value for a feature.
> 
> LOWER_SAFE  - The smaller value is safer
> HIGHER_SAFE - The bigger value is safer
> EXACT       - We can't decide between the two, so a predefined safe_value is used.

The "SCALAR..." etc. in the comment needs updating.

> +#define FTR_STRICT	true
> +#define FTR_NONSTRICT	false

Please add a comment on what STRICT/NONSTRICT mean.

> +static inline u64 ftr_mask(struct arm64_ftr_bits *ftrp)
> +{
> +	return (u64)GENMASK(ftrp->shift + ftrp->width - 1, ftrp->shift);
> +}
> +
> +static inline s64 arm64_ftr_value(struct arm64_ftr_bits *ftrp, u64 val)
> +{
> +	return cpuid_feature_extract_field_width(val, ftrp->shift, ftrp->width);
> +}

Slightly inconsistent naming: since you are prefixing everything with
arm64_, do the same for ftr_mask.

> +static struct arm64_ftr_reg arm64_ftr_regs[] = {
> +
> +	/* This array should be always kept in the ascending order of id  */

Do we have a sanity check somewhere? You could also call sort() on this
array and we wouldn't have to worry.

> +/*
> + * get_arm64_sys_reg - Lookup a feature register entry using its
> + * sys_reg() encoding. With the array arm64_ftr_regs sorted in the
> + * ascending order, we use binary search to find a matching entry.
> + *
> + * returns - Upon success,  matching ftr_reg entry for id.
> + *         - NULL on failure. It is upto the caller to decide
> + *	     the impact of a failure.
> + */
> +static struct arm64_ftr_reg* get_arm64_sys_reg(u32 sys_id)

Nitpick: the * near the function name.

Also rename it to get_arm64_ftr_reg() to match the actual return type.

-- 
Catalin
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/