Re: [PATCH v2 11/20] arm64: capabilities: Add support for features enabled early

[Suzuki K Poulose <Suzuki.Poulose@arm.com>]

On 08/02/18 11:35, Dave Martin wrote:
> On Wed, Feb 07, 2018 at 06:34:37PM +0000, Suzuki K Poulose wrote:
>> On 07/02/18 10:38, Dave Martin wrote:
>>> On Wed, Jan 31, 2018 at 06:27:58PM +0000, Suzuki K Poulose wrote:

>>>>    * 3) Verification: When a CPU is brought online (e.g, by user or by the kernel),
>>>>    *    the kernel should make sure that it is safe to use the CPU, by verifying
>>>> @@ -139,11 +148,22 @@ extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>>>    *
>>>>    *    As explained in (2) above, capabilities could be finalised at different
>>>>    *    points in the execution. Each CPU is verified against the "finalised"
>>>> - *    capabilities and if there is a conflict, the kernel takes an action, based
>>>> - *    on the severity (e.g, a CPU could be prevented from booting or cause a
>>>> - *    kernel panic). The CPU is allowed to "affect" the state of the capability,
>>>> - *    if it has not been finalised already. See section 5 for more details on
>>>> - *    conflicts.
>>>> + *    capabilities.
>>>> + *
>>>> + *	x------------------------------------------------------------------- x
>>>> + *	| Verification:       | Boot CPU | SMP CPUs by kernel | CPUs by user |
>>>> + *	|--------------------------------------------------------------------|
>>>> + *	| Primary boot CPU    |          |                    |              |
>>>> + *	|  capability         |   n      |      y             |       y      |
>>>> + *	|--------------------------------------------------------------------|
>>>> + *	| All others          |   n      |      n             |       y      |
>>>> + *	x--------------------------------------------------------------------x
>>>
>>> Minor clarify nit: it's not obvious that "n" means "no conflict" and "y"
>>> means "conflict".
>>>
>>> Could we have blank cell versus "X" (with a note saying what that
>>> means), or "ok" versus "CONFLICT"?
>>
>> This is not strictly about conflicts, but about what each CPU get
>> verified against.  Since there are multiple stages of "finalisation"
> 
> You're right: I meant something like "potential conflict", but I hadn't
> read the previous paragraph carefully enough and didn't explain what I
> meant very well.
> 
>> for the capabilities, the table shows how the CPUs get verified.
>>
>> Would it help if I changed the description above the table to :
>>
>>   *    As explained in (2) above, capabilities could be finalised at different
>>   *    points in the execution. Each CPU is verified against the "finalised"
>>   *    capabilities. The following table shows, the capabilities verified
>>   *    against each CPU in the system.
>>   *
>>   *      x------------------------------------------------------------------- x
>>   *      | Verified against:   | Boot CPU | SMP CPUs by kernel | CPUs by user |
> 
> I still find it a bit cryptic.
> 
> Would it be simpler just to write this out in prose, with reference to
> the actual capability types?  I feel that things have to be abbreviated
> a bit to fit nicely into the table otherwise.
> 
> What about:
> 
>   * As explained in (2) above, capabilities could be finalised at different
>   * points in the execution, depending on the capability type. Each newly booted
>   * CPU is verified against those capabilities that have been finalised by the
>   * time that CPU boots:
>   *
>   *	* SCOPE_BOOT_CPU: all CPUs are verified against the capability except
>   *	  for the primary boot CPU.
>   *
>   *	* SCOPE_LOCAL_CPU, SCOPE_SYSTEM: all CPUs hotplugged on by the user
>   *	  after kernel boot are verified against the capability.

Sure, looks better.