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@xxxxxxxxxx>
>>>> Signed-off-by: Peng Liang <liangpeng10@xxxxxxxxxx>
>>>> ---
>>>> 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
