| Message ID | 20200917120101.3438389-2-liangpeng10@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | kvm: arm64: emulate ID registers | expand |
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
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? Thanks, Peng
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
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
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)