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
