linux-kernel - Re: [PATCH 01/16] arm64: capabilities: Update prototype for enable call back

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:   Tue, 23 Jan 2018 15:38:37 +0000
From:   Suzuki K Poulose <Suzuki.Poulose@....com>
To:     Dave Martin <Dave.Martin@....com>
Cc:     linux-arm-kernel@...ts.infradead.org, mark.rutland@....com,
        ckadabi@...eaurora.org, ard.biesheuvel@...aro.org,
        marc.zyngier@....com, catalin.marinas@....com, will.deacon@....com,
        linux-kernel@...r.kernel.org, James Morse <james.morse@....com>,
        jnair@...iumnetworks.com, Andre Przywara <andre.przywara@....com>,
        Robin Murphy <robin.murphy@....com>
Subject: Re: [PATCH 01/16] arm64: capabilities: Update prototype for enable
 call back

On 23/01/18 14:52, Dave Martin wrote:
> On Tue, Jan 23, 2018 at 12:27:54PM +0000, Suzuki K Poulose wrote:
>> From: Dave Martin <dave.martin@....com>
>>
>> We issue the enable() call back for all CPU hwcaps capabilities
>> available on the system, on all the CPUs. So far we have ignored
>> the argument passed to the call back, which had a prototype to
>> accept a "void *" for use with on_each_cpu() and later with
>> stop_machine(). However, with commit 0a0d111d40fd1
>> ("arm64: cpufeature: Pass capability structure to ->enable callback"),
>> there are some users of the argument who wants the matching capability
>> struct pointer where there are multiple matching criteria for a single
>> capability. Update the prototype for enable to accept a const pointer.
>>
>> Cc: Will Deacon <will.deacon@....com>
>> Cc: Robin Murphy <robin.murphy@....com>
>> Cc: Catalin Marinas <catalin.marinas@....com>
>> Cc: Mark Rutland <mark.rutland@....com>
>> Cc: Andre Przywara <andre.przywara@....com>
>> Cc: James Morse <james.morse@....com>
>> Reviewed-by: Julien Thierry <julien.thierry@....com>
>> Signed-off-by: Dave Martin <dave.martin@....com>
>> [ Rebased to for-next/core converting more users ]
>> Signed-off-by: Suzuki K Poulose <suzuki.poulose@....com>
>> ---
>>   arch/arm64/include/asm/cpufeature.h |  3 ++-
>>   arch/arm64/include/asm/fpsimd.h     |  4 +++-
>>   arch/arm64/include/asm/processor.h  |  7 ++++---
>>   arch/arm64/kernel/cpu_errata.c      | 14 ++++++--------
>>   arch/arm64/kernel/cpufeature.c      | 16 ++++++++++++----
>>   arch/arm64/kernel/fpsimd.c          |  3 ++-
>>   arch/arm64/kernel/traps.c           |  3 ++-
>>   arch/arm64/mm/fault.c               |  2 +-
>>   8 files changed, 32 insertions(+), 20 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>> index ac67cfc2585a..cefbd685292c 100644
>> --- a/arch/arm64/include/asm/cpufeature.h
>> +++ b/arch/arm64/include/asm/cpufeature.h
>> @@ -97,7 +97,8 @@ struct arm64_cpu_capabilities {
>>   	u16 capability;
>>   	int def_scope;			/* default scope */
>>   	bool (*matches)(const struct arm64_cpu_capabilities *caps, int scope);
>> -	int (*enable)(void *);		/* Called on all active CPUs */
>> +	/* Called on all active CPUs for all  "available" capabilities */
> 
> Nit: Odd spacing?  Also, "available" doesn't really make sense for errata
> workarounds.
> 

Thanks for spotting, will fix it.

> Maybe applicable would be a better word?
> 

There is a subtle difference. If there are two entries for a capability,
with only one of them matches, we end up calling the enable() for both
the entries. "Applicable" could potentially be misunderstood, leading
to assumption that the enable() is called only if that "entry" matches,
which is not true. I accept that "available" doesn't sound any better either.


>> +	int (*enable)(const struct arm64_cpu_capabilities *caps);
> 
> Alternatively, if the comment is liable to be ambiguous, maybe it would
> be better to delete it.  The explicit argument type already makes this
> more self-documenting than previously.

I think we still need to make it clear that the enable is called on
all active CPUs. It is not about the argument anymore.

How about :

/*
  * Called on all active CPUs if the capability associated with
  * this entry is set.
  */


> 
> I don't feel that strongly either way though; probably not worth a
> respin unless you have other things to change.
> 
> Also please note that I didn't test the original patch here (in case
> I didn't point that out already...)

I think I did test it using the HW DBM feature (see the patch at the end).
However, will do some more testing with it. Thanks for mentioning.

Suzuki