Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs

From: Peng Liang <liangpeng10@huawei.com>
To: Andrew Jones <drjones@redhat.com>
Cc: zhang.zhanghailiang@huawei.com, kvm@vger.kernel.org,
	maz@kernel.org, will@kernel.org, kvmarm@lists.cs.columbia.edu
Subject: Re: [RFC v2 1/7] arm64: add a helper function to traverse arm64_ftr_regs
Date: Fri, 18 Sep 2020 19:58:43 +0800	[thread overview]
Message-ID: <a0960559-ff81-cb30-ffa1-4ed1cdae65e7@huawei.com> (raw)
In-Reply-To: <20200918102808.gwpk6ggy36prq7iv@kamzik.brq.redhat.com>

On 9/18/2020 6:28 PM, Andrew Jones wrote:
> On Fri, Sep 18, 2020 at 05:24:27PM +0800, Peng Liang wrote:
>> On 9/18/2020 3:18 PM, Andrew Jones wrote:
>>> On Thu, Sep 17, 2020 at 08:00:55PM +0800, Peng Liang wrote:
>>>> If we want to emulate ID registers, we need to initialize ID registers
>>>> firstly.  This commit is to add a helper function to traverse
>>>> arm64_ftr_regs so that we can initialize ID registers from
>>>> arm64_ftr_regs.
>>>>
>>>> Signed-off-by: zhanghailiang <zhang.zhanghailiang@huawei.com>
>>>> Signed-off-by: Peng Liang <liangpeng10@huawei.com>
>>>> ---
>>>>  arch/arm64/include/asm/cpufeature.h |  2 ++
>>>>  arch/arm64/kernel/cpufeature.c      | 13 +++++++++++++
>>>>  2 files changed, 15 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/include/asm/cpufeature.h b/arch/arm64/include/asm/cpufeature.h
>>>> index 89b4f0142c28..2ba7c4f11d8a 100644
>>>> --- a/arch/arm64/include/asm/cpufeature.h
>>>> +++ b/arch/arm64/include/asm/cpufeature.h
>>>> @@ -79,6 +79,8 @@ struct arm64_ftr_reg {
>>>>  
>>>>  extern struct arm64_ftr_reg arm64_ftr_reg_ctrel0;
>>>>  
>>>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp);
>>>> +
>>>>  /*
>>>>   * CPU capabilities:
>>>>   *
>>>> diff --git a/arch/arm64/kernel/cpufeature.c b/arch/arm64/kernel/cpufeature.c
>>>> index 6424584be01e..698b32705544 100644
>>>> --- a/arch/arm64/kernel/cpufeature.c
>>>> +++ b/arch/arm64/kernel/cpufeature.c
>>>> @@ -1112,6 +1112,19 @@ u64 read_sanitised_ftr_reg(u32 id)
>>>>  	return regp->sys_val;
>>>>  }
>>>>  
>>>> +int arm64_cpu_ftr_regs_traverse(int (*op)(u32, u64, void *), void *argp)
>>>> +{
>>>> +	int i, ret;
>>>> +
>>>> +	for (i = 0; i <  ARRAY_SIZE(arm64_ftr_regs); i++) {
>>>> +		ret = (*op)(arm64_ftr_regs[i].sys_id,
>>>> +			    arm64_ftr_regs[i].reg->sys_val, argp);
>>>> +		if (ret < 0)
>>>> +			return ret;
>>>> +	}
>>>> +	return 0;
>>>> +}
>>>> +
>>>>  #define read_sysreg_case(r)	\
>>>>  	case r:		return read_sysreg_s(r)
>>>>  
>>>> -- 
>>>> 2.26.2
>>>>
>>>
>>> Skimming the rest of the patches to see how this is used I only saw a
>>> single callsite. Why wouldn't we just put this simple for-loop right
>>> there at that callsite? Or, IOW, I think this traverse function should
>>> be dropped.
>>>
>>> Thanks,
>>> drew
>>>
>>> .
>>>
>>
>> arm64_ftr_regs is defined as a static array in arch/arm64/kernel/cpufeature.c,
>> which is not a virtualization-related file.  Putting this simple for-loop
>> right there will make cpufeature.c depend on kvm_host.h.  Is this a good idea?
> 
> Well, the fact that arm64_ftr_regs is static to cpufeature.c is a clue
> that your implementation is likely playing with internal arm64_ftr
> state that it shouldn't be. If there's not an accessor function that
> works for you, then you can try adding one. Providing general functions
> like this, that are effectively just an odd way of removing 'static'
> from arm64_ftr_regs, breaks the encapsulation.
> 
> Thanks,
> drew
> 
> .
> 

I found get_arm64_ftr_reg_nowarn and get_arm64_ftr_reg in cpufeature.c which will
search and return the arm64_ftr_reg* according to the sys_id.  But they are all
static.  Hence, I think cpufeature.c don't want other modules to access the
arm64_ftr_reg*.  So I add arm64_cpu_ftr_regs_traverse to traverse the
arm64_ftr_regs and pass the id and value to op instead of the arm64_ftr_reg*.

Thanks,
Peng
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm