[PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Anshuman Khandual]

There is no way to proceed when requested register could not be searched in
arm64_ftr_reg[]. Requesting for a non present register would be an error as
well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
rather than checking for return value and doing the same in some individual
callers.

But there are some callers that dont BUG_ON() upon search failure. It adds
an argument 'failsafe' that provides required switch between callers based
on whether they could proceed or not.

Cc: Catalin Marinas <catalin.marinas@arm.com>
Cc: Will Deacon <will@kernel.org>
Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
---
Applies on next-20200518 that has recent cpufeature changes from Will.

 arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
index bc5048f152c1..62767cc540c3 100644
--- a/arch/arm64/kernel/cpufeature.c
+++ b/arch/arm64/kernel/cpufeature.c
@@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
  *         - NULL on failure. It is upto the caller to decide
  *	     the impact of a failure.
  */
-static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
+static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
 {
 	const struct __ftr_reg_entry *ret;
 
@@ -568,6 +568,13 @@ static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
 			search_cmp_ftr_reg);
 	if (ret)
 		return ret->reg;
+	/*
+	 * This can not really proceed when the search fails.
+	 * Requesting for a non existent register search will
+	 * also be an error in itself. Error out when not
+	 * called with fail safe request.
+	 */
+	BUG_ON(!failsafe);
 	return NULL;
 }
 
@@ -630,9 +637,7 @@ static void __init init_cpu_ftr_reg(u32 sys_reg, u64 new)
 	u64 valid_mask = 0;
 
 	const struct arm64_ftr_bits *ftrp;
-	struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg);
-
-	BUG_ON(!reg);
+	struct arm64_ftr_reg *reg = get_arm64_ftr_reg(sys_reg, false);
 
 	for (ftrp = reg->ftr_bits; ftrp->width; ftrp++) {
 		u64 ftr_mask = arm64_ftr_mask(ftrp);
@@ -760,9 +765,8 @@ static void update_cpu_ftr_reg(struct arm64_ftr_reg *reg, u64 new)
 
 static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
 {
-	struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
+	struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id, false);
 
-	BUG_ON(!regp);
 	update_cpu_ftr_reg(regp, val);
 	if ((boot & regp->strict_mask) == (val & regp->strict_mask))
 		return 0;
@@ -774,10 +778,7 @@ static int check_update_ftr_reg(u32 sys_id, int cpu, u64 val, u64 boot)
 static void relax_cpu_ftr_reg(u32 sys_id, int field)
 {
 	const struct arm64_ftr_bits *ftrp;
-	struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id);
-
-	if (WARN_ON(!regp))
-		return;
+	struct arm64_ftr_reg *regp = get_arm64_ftr_reg(sys_id, false);
 
 	for (ftrp = regp->ftr_bits; ftrp->width; ftrp++) {
 		if (ftrp->shift == field) {
@@ -959,10 +960,9 @@ void update_cpu_features(int cpu,
 
 u64 read_sanitised_ftr_reg(u32 id)
 {
-	struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id);
+	struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id, false);
 
 	/* We shouldn't get a request for an unsupported register */
-	BUG_ON(!regp);
 	return regp->sys_val;
 }
 
@@ -2565,7 +2565,7 @@ static int emulate_sys_reg(u32 id, u64 *valp)
 	if (sys_reg_CRm(id) == 0)
 		return emulate_id_reg(id, valp);
 
-	regp = get_arm64_ftr_reg(id);
+	regp = get_arm64_ftr_reg(id, true);
 	if (regp)
 		*valp = arm64_ftr_reg_user_value(regp);
 	else
-- 
2.20.1

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Catalin Marinas]

On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
> There is no way to proceed when requested register could not be searched in
> arm64_ftr_reg[]. Requesting for a non present register would be an error as
> well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
> rather than checking for return value and doing the same in some individual
> callers.
> 
> But there are some callers that dont BUG_ON() upon search failure. It adds
> an argument 'failsafe' that provides required switch between callers based
> on whether they could proceed or not.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>

BTW, there should be no empty line between the Cc block and the SoB.

The patch looks fine. Just a note that the patch transforms a current
WARN_ON in a BUG_ON but that's fine by me.

Reviewed-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Will Deacon]

Hi Anshuman,

On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
> There is no way to proceed when requested register could not be searched in
> arm64_ftr_reg[]. Requesting for a non present register would be an error as
> well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
> rather than checking for return value and doing the same in some individual
> callers.
> 
> But there are some callers that dont BUG_ON() upon search failure. It adds
> an argument 'failsafe' that provides required switch between callers based
> on whether they could proceed or not.
> 
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will@kernel.org>
> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: linux-arm-kernel@lists.infradead.org
> Cc: linux-kernel@vger.kernel.org
> 
> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> ---
> Applies on next-20200518 that has recent cpufeature changes from Will.
> 
>  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
>  1 file changed, 13 insertions(+), 13 deletions(-)
> 
> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> index bc5048f152c1..62767cc540c3 100644
> --- a/arch/arm64/kernel/cpufeature.c
> +++ b/arch/arm64/kernel/cpufeature.c
> @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
>   *         - NULL on failure. It is upto the caller to decide
>   *	     the impact of a failure.
>   */
> -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
> +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)

Generally, I'm not a big fan of boolean arguments because they are really
opaque at the callsite. It also seems bogus to me that we don't trust the
caller to pass a valid sys_id, but we trust it to get "failsafe" right,
which seems to mean "I promise to check the result isn't NULL before
dereferencing it."

So I don't see how this patch improves anything. I'd actually be more
inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and
have the callers handle NULL by returning early, getting rid of all the
BUG_ONs in here. Sure, the system might end up in a funny state, but we
WARN()d about it and tried to keep going (and Linus has some strong opinions
on this too).

Will

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Catalin Marinas]

On Wed, May 20, 2020 at 01:20:13PM +0100, Will Deacon wrote:
> On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
> > There is no way to proceed when requested register could not be searched in
> > arm64_ftr_reg[]. Requesting for a non present register would be an error as
> > well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
> > rather than checking for return value and doing the same in some individual
> > callers.
> > 
> > But there are some callers that dont BUG_ON() upon search failure. It adds
> > an argument 'failsafe' that provides required switch between callers based
> > on whether they could proceed or not.
> > 
> > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > Cc: Will Deacon <will@kernel.org>
> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > Cc: Mark Brown <broonie@kernel.org>
> > Cc: linux-arm-kernel@lists.infradead.org
> > Cc: linux-kernel@vger.kernel.org
> > 
> > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > ---
> > Applies on next-20200518 that has recent cpufeature changes from Will.
> > 
> >  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
> >  1 file changed, 13 insertions(+), 13 deletions(-)
> > 
> > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > index bc5048f152c1..62767cc540c3 100644
> > --- a/arch/arm64/kernel/cpufeature.c
> > +++ b/arch/arm64/kernel/cpufeature.c
> > @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
> >   *         - NULL on failure. It is upto the caller to decide
> >   *	     the impact of a failure.
> >   */
> > -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
> > +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
> 
> Generally, I'm not a big fan of boolean arguments because they are really
> opaque at the callsite. It also seems bogus to me that we don't trust the
> caller to pass a valid sys_id, but we trust it to get "failsafe" right,
> which seems to mean "I promise to check the result isn't NULL before
> dereferencing it."
> 
> So I don't see how this patch improves anything. I'd actually be more
> inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and
> have the callers handle NULL by returning early, getting rid of all the
> BUG_ONs in here. Sure, the system might end up in a funny state, but we
> WARN()d about it and tried to keep going (and Linus has some strong opinions
> on this too).

Such WARN can be triggered by the user via emulate_sys_reg(), so we
can't really have it in get_arm64_ftr_reg() without a 'failsafe' option.

-- 
Catalin

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Will Deacon]

On Wed, May 20, 2020 at 04:47:11PM +0100, Catalin Marinas wrote:
> On Wed, May 20, 2020 at 01:20:13PM +0100, Will Deacon wrote:
> > On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
> > > There is no way to proceed when requested register could not be searched in
> > > arm64_ftr_reg[]. Requesting for a non present register would be an error as
> > > well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
> > > rather than checking for return value and doing the same in some individual
> > > callers.
> > > 
> > > But there are some callers that dont BUG_ON() upon search failure. It adds
> > > an argument 'failsafe' that provides required switch between callers based
> > > on whether they could proceed or not.
> > > 
> > > Cc: Catalin Marinas <catalin.marinas@arm.com>
> > > Cc: Will Deacon <will@kernel.org>
> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > > Cc: Mark Brown <broonie@kernel.org>
> > > Cc: linux-arm-kernel@lists.infradead.org
> > > Cc: linux-kernel@vger.kernel.org
> > > 
> > > Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > > ---
> > > Applies on next-20200518 that has recent cpufeature changes from Will.
> > > 
> > >  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
> > >  1 file changed, 13 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > > index bc5048f152c1..62767cc540c3 100644
> > > --- a/arch/arm64/kernel/cpufeature.c
> > > +++ b/arch/arm64/kernel/cpufeature.c
> > > @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
> > >   *         - NULL on failure. It is upto the caller to decide
> > >   *	     the impact of a failure.
> > >   */
> > > -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
> > > +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
> > 
> > Generally, I'm not a big fan of boolean arguments because they are really
> > opaque at the callsite. It also seems bogus to me that we don't trust the
> > caller to pass a valid sys_id, but we trust it to get "failsafe" right,
> > which seems to mean "I promise to check the result isn't NULL before
> > dereferencing it."
> > 
> > So I don't see how this patch improves anything. I'd actually be more
> > inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and
> > have the callers handle NULL by returning early, getting rid of all the
> > BUG_ONs in here. Sure, the system might end up in a funny state, but we
> > WARN()d about it and tried to keep going (and Linus has some strong opinions
> > on this too).
> 
> Such WARN can be triggered by the user via emulate_sys_reg(), so we
> can't really have it in get_arm64_ftr_reg() without a 'failsafe' option.

Ah yes, that would be bad. In which case, I don't think the existing code
should change.

Will

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Anshuman Khandual]

On 05/20/2020 05:50 PM, Will Deacon wrote:
> Hi Anshuman,
> 
> On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
>> There is no way to proceed when requested register could not be searched in
>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
>> well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
>> rather than checking for return value and doing the same in some individual
>> callers.
>>
>> But there are some callers that dont BUG_ON() upon search failure. It adds
>> an argument 'failsafe' that provides required switch between callers based
>> on whether they could proceed or not.
>>
>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>> Cc: Will Deacon <will@kernel.org>
>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>> Cc: Mark Brown <broonie@kernel.org>
>> Cc: linux-arm-kernel@lists.infradead.org
>> Cc: linux-kernel@vger.kernel.org
>>
>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>> ---
>> Applies on next-20200518 that has recent cpufeature changes from Will.
>>
>>  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>
>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>> index bc5048f152c1..62767cc540c3 100644
>> --- a/arch/arm64/kernel/cpufeature.c
>> +++ b/arch/arm64/kernel/cpufeature.c
>> @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
>>   *         - NULL on failure. It is upto the caller to decide
>>   *	     the impact of a failure.
>>   */
>> -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
>> +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
> 
> Generally, I'm not a big fan of boolean arguments because they are really
> opaque at the callsite. It also seems bogus to me that we don't trust the

If preferred, we could replace with an enum variable here with some
more context e.g

enum ftr_reg_search {
	FTR_REG_SEARCH_SAFE,
	FTR_REG_SEARCH_UNSAFE,
};

> caller to pass a valid sys_id, but we trust it to get "failsafe" right,

If we really trust the callers, then why BUG_ON() checks are present in
the first place. Because it is always prudent to protect against the
unexpected.

> which seems to mean "I promise to check the result isn't NULL before
> dereferencing it."

Not sure I got this. Do you mean all the present BUG_ON() are trying to
check that returned arm64_ftr_reg is valid before dereferencing it ? If
there is real trust on the callers that a non present sys_id will never
get requested, then all present BUG_ON() instances should never be there.

Either we trust the callers - drop all BUG_ON() and WARN_ON() instances
or we dont - consolidate BUG_ON() and WARN_ON() instances appropriately.

> 
> So I don't see how this patch improves anything. I'd actually be more

It consolidates multiple BUG_ON() in various callers which are not really
required. Code consolidation and reduction especially BUG_ON() instances,
is invariably a good thing.

> inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and

AFAICS in emulate_sys_reg() where the user can send non-present sys_id
registers that eventually gets emulated, should not expect an WARN_ON()
as it did not do anything wrong.

> have the callers handle NULL by returning early, getting rid of all the
> BUG_ONs in here. Sure, the system might end up in a funny state, but we
> WARN()d about it and tried to keep going (and Linus has some strong opinions
> on this too).

Sure, we could go with an WARN_ON() instead, if acceptable and preferred.

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Anshuman Khandual]

On 05/20/2020 11:09 PM, Will Deacon wrote:
> On Wed, May 20, 2020 at 04:47:11PM +0100, Catalin Marinas wrote:
>> On Wed, May 20, 2020 at 01:20:13PM +0100, Will Deacon wrote:
>>> On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
>>>> There is no way to proceed when requested register could not be searched in
>>>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
>>>> well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
>>>> rather than checking for return value and doing the same in some individual
>>>> callers.
>>>>
>>>> But there are some callers that dont BUG_ON() upon search failure. It adds
>>>> an argument 'failsafe' that provides required switch between callers based
>>>> on whether they could proceed or not.
>>>>
>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>> Cc: Will Deacon <will@kernel.org>
>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>> Cc: Mark Brown <broonie@kernel.org>
>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>>
>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>> ---
>>>> Applies on next-20200518 that has recent cpufeature changes from Will.
>>>>
>>>>  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
>>>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index bc5048f152c1..62767cc540c3 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
>>>>   *         - NULL on failure. It is upto the caller to decide
>>>>   *	     the impact of a failure.
>>>>   */
>>>> -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
>>>> +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
>>>
>>> Generally, I'm not a big fan of boolean arguments because they are really
>>> opaque at the callsite. It also seems bogus to me that we don't trust the
>>> caller to pass a valid sys_id, but we trust it to get "failsafe" right,
>>> which seems to mean "I promise to check the result isn't NULL before
>>> dereferencing it."
>>>
>>> So I don't see how this patch improves anything. I'd actually be more
>>> inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and
>>> have the callers handle NULL by returning early, getting rid of all the
>>> BUG_ONs in here. Sure, the system might end up in a funny state, but we
>>> WARN()d about it and tried to keep going (and Linus has some strong opinions
>>> on this too).
>>
>> Such WARN can be triggered by the user via emulate_sys_reg(), so we
>> can't really have it in get_arm64_ftr_reg() without a 'failsafe' option.
> 
> Ah yes, that would be bad. In which case, I don't think the existing code
> should change.

The existing code has BUG_ON() in three different callers doing exactly the
same thing that can easily be taken care in get_arm64_ftr_reg() itself. As
mentioned before an enum variable (as preferred - over a bool) can still
preserve the existing behavior for emulate_sys_reg().

IMHO these are very good reasons for us to change the code which will make
it cleaner while also removing three redundant BUG_ON() instances. Hence I
will request you to please reconsider this proposal.

- Anshuman

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Will Deacon]

On Thu, May 21, 2020 at 08:45:38AM +0530, Anshuman Khandual wrote:
> 
> 
> On 05/20/2020 11:09 PM, Will Deacon wrote:
> > On Wed, May 20, 2020 at 04:47:11PM +0100, Catalin Marinas wrote:
> >> On Wed, May 20, 2020 at 01:20:13PM +0100, Will Deacon wrote:
> >>> On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
> >>>> There is no way to proceed when requested register could not be searched in
> >>>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
> >>>> well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
> >>>> rather than checking for return value and doing the same in some individual
> >>>> callers.
> >>>>
> >>>> But there are some callers that dont BUG_ON() upon search failure. It adds
> >>>> an argument 'failsafe' that provides required switch between callers based
> >>>> on whether they could proceed or not.
> >>>>
> >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> >>>> Cc: Will Deacon <will@kernel.org>
> >>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> >>>> Cc: Mark Brown <broonie@kernel.org>
> >>>> Cc: linux-arm-kernel@lists.infradead.org
> >>>> Cc: linux-kernel@vger.kernel.org
> >>>>
> >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> >>>> ---
> >>>> Applies on next-20200518 that has recent cpufeature changes from Will.
> >>>>
> >>>>  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
> >>>>  1 file changed, 13 insertions(+), 13 deletions(-)
> >>>>
> >>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> >>>> index bc5048f152c1..62767cc540c3 100644
> >>>> --- a/arch/arm64/kernel/cpufeature.c
> >>>> +++ b/arch/arm64/kernel/cpufeature.c
> >>>> @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
> >>>>   *         - NULL on failure. It is upto the caller to decide
> >>>>   *	     the impact of a failure.
> >>>>   */
> >>>> -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
> >>>> +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
> >>>
> >>> Generally, I'm not a big fan of boolean arguments because they are really
> >>> opaque at the callsite. It also seems bogus to me that we don't trust the
> >>> caller to pass a valid sys_id, but we trust it to get "failsafe" right,
> >>> which seems to mean "I promise to check the result isn't NULL before
> >>> dereferencing it."
> >>>
> >>> So I don't see how this patch improves anything. I'd actually be more
> >>> inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and
> >>> have the callers handle NULL by returning early, getting rid of all the
> >>> BUG_ONs in here. Sure, the system might end up in a funny state, but we
> >>> WARN()d about it and tried to keep going (and Linus has some strong opinions
> >>> on this too).
> >>
> >> Such WARN can be triggered by the user via emulate_sys_reg(), so we
> >> can't really have it in get_arm64_ftr_reg() without a 'failsafe' option.
> > 
> > Ah yes, that would be bad. In which case, I don't think the existing code
> > should change.
> 
> The existing code has BUG_ON() in three different callers doing exactly the
> same thing that can easily be taken care in get_arm64_ftr_reg() itself. As
> mentioned before an enum variable (as preferred - over a bool) can still
> preserve the existing behavior for emulate_sys_reg().
> 
> IMHO these are very good reasons for us to change the code which will make
> it cleaner while also removing three redundant BUG_ON() instances. Hence I
> will request you to please reconsider this proposal.

Hmm, then how about trying my proposal with the WARN_ON(), but having a
get_arm64_ftr_reg_nowarn() variant for the user emulation case?

Will

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Catalin Marinas]

On Thu, May 21, 2020 at 05:22:15PM +0100, Will Deacon wrote:
> On Thu, May 21, 2020 at 08:45:38AM +0530, Anshuman Khandual wrote:
> > On 05/20/2020 11:09 PM, Will Deacon wrote:
> > > On Wed, May 20, 2020 at 04:47:11PM +0100, Catalin Marinas wrote:
> > >> On Wed, May 20, 2020 at 01:20:13PM +0100, Will Deacon wrote:
> > >>> On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
> > >>>> There is no way to proceed when requested register could not be searched in
> > >>>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
> > >>>> well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
> > >>>> rather than checking for return value and doing the same in some individual
> > >>>> callers.
> > >>>>
> > >>>> But there are some callers that dont BUG_ON() upon search failure. It adds
> > >>>> an argument 'failsafe' that provides required switch between callers based
> > >>>> on whether they could proceed or not.
> > >>>>
> > >>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
> > >>>> Cc: Will Deacon <will@kernel.org>
> > >>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
> > >>>> Cc: Mark Brown <broonie@kernel.org>
> > >>>> Cc: linux-arm-kernel@lists.infradead.org
> > >>>> Cc: linux-kernel@vger.kernel.org
> > >>>>
> > >>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
> > >>>> ---
> > >>>> Applies on next-20200518 that has recent cpufeature changes from Will.
> > >>>>
> > >>>>  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
> > >>>>  1 file changed, 13 insertions(+), 13 deletions(-)
> > >>>>
> > >>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
> > >>>> index bc5048f152c1..62767cc540c3 100644
> > >>>> --- a/arch/arm64/kernel/cpufeature.c
> > >>>> +++ b/arch/arm64/kernel/cpufeature.c
> > >>>> @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
> > >>>>   *         - NULL on failure. It is upto the caller to decide
> > >>>>   *	     the impact of a failure.
> > >>>>   */
> > >>>> -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
> > >>>> +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
> > >>>
> > >>> Generally, I'm not a big fan of boolean arguments because they are really
> > >>> opaque at the callsite. It also seems bogus to me that we don't trust the
> > >>> caller to pass a valid sys_id, but we trust it to get "failsafe" right,
> > >>> which seems to mean "I promise to check the result isn't NULL before
> > >>> dereferencing it."
> > >>>
> > >>> So I don't see how this patch improves anything. I'd actually be more
> > >>> inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and
> > >>> have the callers handle NULL by returning early, getting rid of all the
> > >>> BUG_ONs in here. Sure, the system might end up in a funny state, but we
> > >>> WARN()d about it and tried to keep going (and Linus has some strong opinions
> > >>> on this too).
> > >>
> > >> Such WARN can be triggered by the user via emulate_sys_reg(), so we
> > >> can't really have it in get_arm64_ftr_reg() without a 'failsafe' option.
> > > 
> > > Ah yes, that would be bad. In which case, I don't think the existing code
> > > should change.
> > 
> > The existing code has BUG_ON() in three different callers doing exactly the
> > same thing that can easily be taken care in get_arm64_ftr_reg() itself. As
> > mentioned before an enum variable (as preferred - over a bool) can still
> > preserve the existing behavior for emulate_sys_reg().
> > 
> > IMHO these are very good reasons for us to change the code which will make
> > it cleaner while also removing three redundant BUG_ON() instances. Hence I
> > will request you to please reconsider this proposal.
> 
> Hmm, then how about trying my proposal with the WARN_ON(), but having a
> get_arm64_ftr_reg_nowarn() variant for the user emulation case?

That works for me, get_arm64_ftr_reg() would be a wrapper over the
_nowarn function with the added WARN_ON.

read_sanitised_ftr_reg() would need to return something though. Would
all 0s be ok? I think it works as long as we don't have negative CPUID
fields.

-- 
Catalin

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Anshuman Khandual]

On 05/21/2020 10:29 PM, Catalin Marinas wrote:
> On Thu, May 21, 2020 at 05:22:15PM +0100, Will Deacon wrote:
>> On Thu, May 21, 2020 at 08:45:38AM +0530, Anshuman Khandual wrote:
>>> On 05/20/2020 11:09 PM, Will Deacon wrote:
>>>> On Wed, May 20, 2020 at 04:47:11PM +0100, Catalin Marinas wrote:
>>>>> On Wed, May 20, 2020 at 01:20:13PM +0100, Will Deacon wrote:
>>>>>> On Wed, May 20, 2020 at 06:52:54AM +0530, Anshuman Khandual wrote:
>>>>>>> There is no way to proceed when requested register could not be searched in
>>>>>>> arm64_ftr_reg[]. Requesting for a non present register would be an error as
>>>>>>> well. Hence lets just BUG_ON() when the search fails in get_arm64_ftr_reg()
>>>>>>> rather than checking for return value and doing the same in some individual
>>>>>>> callers.
>>>>>>>
>>>>>>> But there are some callers that dont BUG_ON() upon search failure. It adds
>>>>>>> an argument 'failsafe' that provides required switch between callers based
>>>>>>> on whether they could proceed or not.
>>>>>>>
>>>>>>> Cc: Catalin Marinas <catalin.marinas@arm.com>
>>>>>>> Cc: Will Deacon <will@kernel.org>
>>>>>>> Cc: Suzuki K Poulose <suzuki.poulose@arm.com>
>>>>>>> Cc: Mark Brown <broonie@kernel.org>
>>>>>>> Cc: linux-arm-kernel@lists.infradead.org
>>>>>>> Cc: linux-kernel@vger.kernel.org
>>>>>>>
>>>>>>> Signed-off-by: Anshuman Khandual <anshuman.khandual@arm.com>
>>>>>>> ---
>>>>>>> Applies on next-20200518 that has recent cpufeature changes from Will.
>>>>>>>
>>>>>>>  arch/arm64/kernel/cpufeature.c | 26 +++++++++++++-------------
>>>>>>>  1 file changed, 13 insertions(+), 13 deletions(-)
>>>>>>>
>>>>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>>>>> index bc5048f152c1..62767cc540c3 100644
>>>>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>>>>> @@ -557,7 +557,7 @@ static int search_cmp_ftr_reg(const void *id, const void *regp)
>>>>>>>   *         - NULL on failure. It is upto the caller to decide
>>>>>>>   *	     the impact of a failure.
>>>>>>>   */
>>>>>>> -static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id)
>>>>>>> +static struct arm64_ftr_reg *get_arm64_ftr_reg(u32 sys_id, bool failsafe)
>>>>>>
>>>>>> Generally, I'm not a big fan of boolean arguments because they are really
>>>>>> opaque at the callsite. It also seems bogus to me that we don't trust the
>>>>>> caller to pass a valid sys_id, but we trust it to get "failsafe" right,
>>>>>> which seems to mean "I promise to check the result isn't NULL before
>>>>>> dereferencing it."
>>>>>>
>>>>>> So I don't see how this patch improves anything. I'd actually be more
>>>>>> inclined to stick a WARN() in get_arm64_ftr_reg() when it returns NULL and
>>>>>> have the callers handle NULL by returning early, getting rid of all the
>>>>>> BUG_ONs in here. Sure, the system might end up in a funny state, but we
>>>>>> WARN()d about it and tried to keep going (and Linus has some strong opinions
>>>>>> on this too).
>>>>>
>>>>> Such WARN can be triggered by the user via emulate_sys_reg(), so we
>>>>> can't really have it in get_arm64_ftr_reg() without a 'failsafe' option.
>>>>
>>>> Ah yes, that would be bad. In which case, I don't think the existing code
>>>> should change.
>>>
>>> The existing code has BUG_ON() in three different callers doing exactly the
>>> same thing that can easily be taken care in get_arm64_ftr_reg() itself. As
>>> mentioned before an enum variable (as preferred - over a bool) can still
>>> preserve the existing behavior for emulate_sys_reg().
>>>
>>> IMHO these are very good reasons for us to change the code which will make
>>> it cleaner while also removing three redundant BUG_ON() instances. Hence I
>>> will request you to please reconsider this proposal.
>>
>> Hmm, then how about trying my proposal with the WARN_ON(), but having a
>> get_arm64_ftr_reg_nowarn() variant for the user emulation case?
> 
> That works for me, get_arm64_ftr_reg() would be a wrapper over the
> _nowarn function with the added WARN_ON.

Sure, will do.

> 
> read_sanitised_ftr_reg() would need to return something though. Would
> all 0s be ok? I think it works as long as we don't have negative CPUID
> fields.

Just trying to understand. If get_arm64_ftr_reg() returns NULL, then
read_sanitised_ftr_reg() should also return 0 for that non existent
register (already been warned in get_arm64_ftr_reg).

@@ -961,8 +972,8 @@ u64 read_sanitised_ftr_reg(u32 id)
 {
        struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id);

-       /* We shouldn't get a request for an unsupported register */
-       BUG_ON(!regp);
+       if (!regp)
+               return 0;
        return regp->sys_val;
 }

Re: [PATCH] arm64/cpufeature: Move BUG_ON() inside get_arm64_ftr_reg()

[Catalin Marinas]

On Mon, May 25, 2020 at 05:22:23AM +0530, Anshuman Khandual wrote:
> On 05/21/2020 10:29 PM, Catalin Marinas wrote:
> > On Thu, May 21, 2020 at 05:22:15PM +0100, Will Deacon wrote:
> >> On Thu, May 21, 2020 at 08:45:38AM +0530, Anshuman Khandual wrote:
> >>> The existing code has BUG_ON() in three different callers doing exactly the
> >>> same thing that can easily be taken care in get_arm64_ftr_reg() itself. As
> >>> mentioned before an enum variable (as preferred - over a bool) can still
> >>> preserve the existing behavior for emulate_sys_reg().
> >>>
> >>> IMHO these are very good reasons for us to change the code which will make
> >>> it cleaner while also removing three redundant BUG_ON() instances. Hence I
> >>> will request you to please reconsider this proposal.
> >>
> >> Hmm, then how about trying my proposal with the WARN_ON(), but having a
> >> get_arm64_ftr_reg_nowarn() variant for the user emulation case?
[...]
> > read_sanitised_ftr_reg() would need to return something though. Would
> > all 0s be ok? I think it works as long as we don't have negative CPUID
> > fields.
> 
> Just trying to understand. If get_arm64_ftr_reg() returns NULL, then
> read_sanitised_ftr_reg() should also return 0 for that non existent
> register (already been warned in get_arm64_ftr_reg).
> 
> @@ -961,8 +972,8 @@ u64 read_sanitised_ftr_reg(u32 id)
>  {
>         struct arm64_ftr_reg *regp = get_arm64_ftr_reg(id);
> 
> -       /* We shouldn't get a request for an unsupported register */
> -       BUG_ON(!regp);
> +       if (!regp)
> +               return 0;
>         return regp->sys_val;
>  }

Yes, as long as we also get the WARN_ON().

-- 
Catalin