diff mbox series

KVM: arm64: Fix the name of sys_reg_desc related to PMU

Message ID 1689148505-13914-1-git-send-email-chenxiang66@hisilicon.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Fix the name of sys_reg_desc related to PMU | expand

Commit Message

chenxiang July 12, 2023, 7:55 a.m. UTC 
From: Xiang Chen <chenxiang66@hisilicon.com>

For those PMU system registers defined in sys_reg_descs[], use macro
PMU_SYS_REG() / PMU_PMEVCNTR_EL0 / PMU_PMEVTYPER_EL0 to define them, and
later two macros call macro PMU_SYS_REG() actually.
Currently the input parameter of PMU_SYS_REG() is other macro which is
calculation formula of the value of system registers, so for example, if 
we want to "SYS_PMINTENSET_EL1" as the name of sys register, actually 
the name will be as following:
(((3) << 19) | ((0) << 16) | ((9) << 12) | ((14) << 8) | ((1) << 5))

To fix the issue, use the name as a input parameter of PMU_SYS_REG like
MTE_REG or EL2_REG.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
---
 arch/arm64/kvm/sys_regs.c | 41 +++++++++++++++++++++--------------------
 1 file changed, 21 insertions(+), 20 deletions(-)

Comments

Marc Zyngier July 12, 2023, 8:36 a.m. UTC | #1 
On Wed, 12 Jul 2023 08:55:05 +0100,
chenxiang <chenxiang66@hisilicon.com> wrote:
> 
> From: Xiang Chen <chenxiang66@hisilicon.com>
> 
> For those PMU system registers defined in sys_reg_descs[], use macro
> PMU_SYS_REG() / PMU_PMEVCNTR_EL0 / PMU_PMEVTYPER_EL0 to define them, and
> later two macros call macro PMU_SYS_REG() actually.
> Currently the input parameter of PMU_SYS_REG() is other macro which is
> calculation formula of the value of system registers, so for example, if 
> we want to "SYS_PMINTENSET_EL1" as the name of sys register, actually 
> the name will be as following:
> (((3) << 19) | ((0) << 16) | ((9) << 12) | ((14) << 8) | ((1) << 5))
> 
> To fix the issue, use the name as a input parameter of PMU_SYS_REG like
> MTE_REG or EL2_REG.

Why is the name relevant? Is this related to tracing?

	M.
chenxiang July 13, 2023, 2:35 a.m. UTC | #2 
Hi Marc,


在 2023/7/12 星期三 16:36, Marc Zyngier 写道:
> On Wed, 12 Jul 2023 08:55:05 +0100,
> chenxiang <chenxiang66@hisilicon.com> wrote:
>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>
>> For those PMU system registers defined in sys_reg_descs[], use macro
>> PMU_SYS_REG() / PMU_PMEVCNTR_EL0 / PMU_PMEVTYPER_EL0 to define them, and
>> later two macros call macro PMU_SYS_REG() actually.
>> Currently the input parameter of PMU_SYS_REG() is other macro which is
>> calculation formula of the value of system registers, so for example, if
>> we want to "SYS_PMINTENSET_EL1" as the name of sys register, actually
>> the name will be as following:
>> (((3) << 19) | ((0) << 16) | ((9) << 12) | ((14) << 8) | ((1) << 5))
>>
>> To fix the issue, use the name as a input parameter of PMU_SYS_REG like
>> MTE_REG or EL2_REG.
> Why is the name relevant? Is this related to tracing?
I think It is not related to tracing. I find the issue when i want to 
know which system reigsters are set
when on live migration and printk the name of sys_reg_desc in function 
kvm_sys_reg_set_user,
find the name of some registers are abnormal as followings:

[  227.145540] kvm_sys_reg_set_user 3427:SYS_FAR_EL1
[  227.158057] kvm_sys_reg_set_user 3427:SYS_PAR_EL1
[  227.170574] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) | 
((9) << 12) | ((14) << 8) | ((1) << 5))
[  227.188037] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) | 
((9) << 12) | ((14) << 8) | ((2) << 5))
[  227.205511] kvm_sys_reg_set_user 3427:SYS_MAIR_EL1
[  227.218117] kvm_sys_reg_set_user 3427:SYS_PIRE0_EL1

I think it is caused by the macro PMU_SYS_REG().  For example 
PMU_SYS_REG(SYS_PMINTENSET_EL1),
#define SYS_PMINTENSET_EL1 sys_reg(3, 0, 9, 14, 1)
#define sys_reg(op0, op1, crn, crm, op2) (((op0) << OP0_shift) | ((op1) 
<< OP1_shift) | ((crn) << CRn_shift) | ((crm) << CRm_shift) | ((op2) << 
OP2_shift))

So SYS_PMINTENSET_EL1 is equal to (((3) << 19) | ((0) << 16) | ((9) << 
12) | ((14) << 8) | ((1) << 5)), and PMU_SYS_REG(SYS_PMINTENSET_EL1) calls
SYS_DESC() which uses the input parameter as the name, so the name of 
SYS_PMINTENSET_EL1 becomes  (((3) << 19) | ((0) << 16) | ((9) << 12) | 
((14) << 8) | ((1) << 5)).
Marc Zyngier July 13, 2023, 6:56 a.m. UTC | #3 
On Thu, 13 Jul 2023 03:35:39 +0100,
"chenxiang (M)" <chenxiang66@hisilicon.com> wrote:
> 
> Hi Marc,
> 
> 
> 在 2023/7/12 星期三 16:36, Marc Zyngier 写道:
> > On Wed, 12 Jul 2023 08:55:05 +0100,
> > chenxiang <chenxiang66@hisilicon.com> wrote:
> >> From: Xiang Chen <chenxiang66@hisilicon.com>
> >> 
> >> For those PMU system registers defined in sys_reg_descs[], use macro
> >> PMU_SYS_REG() / PMU_PMEVCNTR_EL0 / PMU_PMEVTYPER_EL0 to define them, and
> >> later two macros call macro PMU_SYS_REG() actually.
> >> Currently the input parameter of PMU_SYS_REG() is other macro which is
> >> calculation formula of the value of system registers, so for example, if
> >> we want to "SYS_PMINTENSET_EL1" as the name of sys register, actually
> >> the name will be as following:
> >> (((3) << 19) | ((0) << 16) | ((9) << 12) | ((14) << 8) | ((1) << 5))
> >> 
> >> To fix the issue, use the name as a input parameter of PMU_SYS_REG like
> >> MTE_REG or EL2_REG.
> > Why is the name relevant? Is this related to tracing?
> I think It is not related to tracing. I find the issue when i want to
> know which system reigsters are set
> when on live migration and printk the name of sys_reg_desc in function
> kvm_sys_reg_set_user,
> find the name of some registers are abnormal as followings:
> 
> [  227.145540] kvm_sys_reg_set_user 3427:SYS_FAR_EL1
> [  227.158057] kvm_sys_reg_set_user 3427:SYS_PAR_EL1
> [  227.170574] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
> ((9) << 12) | ((14) << 8) | ((1) << 5))
> [  227.188037] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
> ((9) << 12) | ((14) << 8) | ((2) << 5))
> [  227.205511] kvm_sys_reg_set_user 3427:SYS_MAIR_EL1
> [  227.218117] kvm_sys_reg_set_user 3427:SYS_PIRE0_EL1

So what is this if not some form of tracing?

I agree that it would be good to fix it, at least because the register
name is smaller than the encoding, but the commit message should at
least mention what this is used for.

Thanks,

	M.
chenxiang July 13, 2023, 9:10 a.m. UTC | #4 
Hi Marc,


在 2023/7/13 星期四 14:56, Marc Zyngier 写道:
> On Thu, 13 Jul 2023 03:35:39 +0100,
> "chenxiang (M)" <chenxiang66@hisilicon.com> wrote:
>> Hi Marc,
>>
>>
>> 在 2023/7/12 星期三 16:36, Marc Zyngier 写道:
>>> On Wed, 12 Jul 2023 08:55:05 +0100,
>>> chenxiang <chenxiang66@hisilicon.com> wrote:
>>>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>>>
>>>> For those PMU system registers defined in sys_reg_descs[], use macro
>>>> PMU_SYS_REG() / PMU_PMEVCNTR_EL0 / PMU_PMEVTYPER_EL0 to define them, and
>>>> later two macros call macro PMU_SYS_REG() actually.
>>>> Currently the input parameter of PMU_SYS_REG() is other macro which is
>>>> calculation formula of the value of system registers, so for example, if
>>>> we want to "SYS_PMINTENSET_EL1" as the name of sys register, actually
>>>> the name will be as following:
>>>> (((3) << 19) | ((0) << 16) | ((9) << 12) | ((14) << 8) | ((1) << 5))
>>>>
>>>> To fix the issue, use the name as a input parameter of PMU_SYS_REG like
>>>> MTE_REG or EL2_REG.
>>> Why is the name relevant? Is this related to tracing?
>> I think It is not related to tracing. I find the issue when i want to
>> know which system reigsters are set
>> when on live migration and printk the name of sys_reg_desc in function
>> kvm_sys_reg_set_user,
>> find the name of some registers are abnormal as followings:
>>
>> [  227.145540] kvm_sys_reg_set_user 3427:SYS_FAR_EL1
>> [  227.158057] kvm_sys_reg_set_user 3427:SYS_PAR_EL1
>> [  227.170574] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
>> ((9) << 12) | ((14) << 8) | ((1) << 5))
>> [  227.188037] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
>> ((9) << 12) | ((14) << 8) | ((2) << 5))
>> [  227.205511] kvm_sys_reg_set_user 3427:SYS_MAIR_EL1
>> [  227.218117] kvm_sys_reg_set_user 3427:SYS_PIRE0_EL1
> So what is this if not some form of tracing?

Ah, i was misunderstood your question. I thought you aksed whether it is 
related to kernel tracing
  (which is already existed in kernel) such as tracepoint or debugfs.

>
> I agree that it would be good to fix it, at least because the register
> name is smaller than the encoding, but the commit message should at
> least mention what this is used for.

Ok, i will modify commit message in next version.

>
> Thanks,
>
> 	M.
>
Marc Zyngier July 13, 2023, 9:40 a.m. UTC | #5 
On Thu, 13 Jul 2023 10:10:01 +0100,
"chenxiang (M)" <chenxiang66@hisilicon.com> wrote:
> 
> Hi Marc,
> 
> 
> 在 2023/7/13 星期四 14:56, Marc Zyngier 写道:
> > On Thu, 13 Jul 2023 03:35:39 +0100,
> > "chenxiang (M)" <chenxiang66@hisilicon.com> wrote:
> >> Hi Marc,
> >> 
> >> 
> >> 在 2023/7/12 星期三 16:36, Marc Zyngier 写道:
> >>> On Wed, 12 Jul 2023 08:55:05 +0100,
> >>> chenxiang <chenxiang66@hisilicon.com> wrote:
> >>>> From: Xiang Chen <chenxiang66@hisilicon.com>
> >>>> 
> >>>> For those PMU system registers defined in sys_reg_descs[], use macro
> >>>> PMU_SYS_REG() / PMU_PMEVCNTR_EL0 / PMU_PMEVTYPER_EL0 to define them, and
> >>>> later two macros call macro PMU_SYS_REG() actually.
> >>>> Currently the input parameter of PMU_SYS_REG() is other macro which is
> >>>> calculation formula of the value of system registers, so for example, if
> >>>> we want to "SYS_PMINTENSET_EL1" as the name of sys register, actually
> >>>> the name will be as following:
> >>>> (((3) << 19) | ((0) << 16) | ((9) << 12) | ((14) << 8) | ((1) << 5))
> >>>> 
> >>>> To fix the issue, use the name as a input parameter of PMU_SYS_REG like
> >>>> MTE_REG or EL2_REG.
> >>> Why is the name relevant? Is this related to tracing?
> >> I think It is not related to tracing. I find the issue when i want to
> >> know which system reigsters are set
> >> when on live migration and printk the name of sys_reg_desc in function
> >> kvm_sys_reg_set_user,
> >> find the name of some registers are abnormal as followings:
> >> 
> >> [  227.145540] kvm_sys_reg_set_user 3427:SYS_FAR_EL1
> >> [  227.158057] kvm_sys_reg_set_user 3427:SYS_PAR_EL1
> >> [  227.170574] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
> >> ((9) << 12) | ((14) << 8) | ((1) << 5))
> >> [  227.188037] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
> >> ((9) << 12) | ((14) << 8) | ((2) << 5))
> >> [  227.205511] kvm_sys_reg_set_user 3427:SYS_MAIR_EL1
> >> [  227.218117] kvm_sys_reg_set_user 3427:SYS_PIRE0_EL1
> > So what is this if not some form of tracing?
> 
> Ah, i was misunderstood your question. I thought you aksed whether
> it is related to kernel tracing (which is already existed in kernel)
> such as tracepoint or debugfs.

But that's my point: tracepoints such as kvm_sys_access already output
the name (as well as the encoding), and are likely to be affected by
this problem.

And that's a much more interesting justification for this change.

Thanks,

	M.
chenxiang July 14, 2023, 3:13 a.m. UTC | #6 
Hi Marc,


在 2023/7/13 星期四 17:40, Marc Zyngier 写道:
> On Thu, 13 Jul 2023 10:10:01 +0100,
> "chenxiang (M)" <chenxiang66@hisilicon.com> wrote:
>> Hi Marc,
>>
>>
>> 在 2023/7/13 星期四 14:56, Marc Zyngier 写道:
>>> On Thu, 13 Jul 2023 03:35:39 +0100,
>>> "chenxiang (M)" <chenxiang66@hisilicon.com> wrote:
>>>> Hi Marc,
>>>>
>>>>
>>>> 在 2023/7/12 星期三 16:36, Marc Zyngier 写道:
>>>>> On Wed, 12 Jul 2023 08:55:05 +0100,
>>>>> chenxiang <chenxiang66@hisilicon.com> wrote:
>>>>>> From: Xiang Chen <chenxiang66@hisilicon.com>
>>>>>>
>>>>>> For those PMU system registers defined in sys_reg_descs[], use macro
>>>>>> PMU_SYS_REG() / PMU_PMEVCNTR_EL0 / PMU_PMEVTYPER_EL0 to define them, and
>>>>>> later two macros call macro PMU_SYS_REG() actually.
>>>>>> Currently the input parameter of PMU_SYS_REG() is other macro which is
>>>>>> calculation formula of the value of system registers, so for example, if
>>>>>> we want to "SYS_PMINTENSET_EL1" as the name of sys register, actually
>>>>>> the name will be as following:
>>>>>> (((3) << 19) | ((0) << 16) | ((9) << 12) | ((14) << 8) | ((1) << 5))
>>>>>>
>>>>>> To fix the issue, use the name as a input parameter of PMU_SYS_REG like
>>>>>> MTE_REG or EL2_REG.
>>>>> Why is the name relevant? Is this related to tracing?
>>>> I think It is not related to tracing. I find the issue when i want to
>>>> know which system reigsters are set
>>>> when on live migration and printk the name of sys_reg_desc in function
>>>> kvm_sys_reg_set_user,
>>>> find the name of some registers are abnormal as followings:
>>>>
>>>> [  227.145540] kvm_sys_reg_set_user 3427:SYS_FAR_EL1
>>>> [  227.158057] kvm_sys_reg_set_user 3427:SYS_PAR_EL1
>>>> [  227.170574] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
>>>> ((9) << 12) | ((14) << 8) | ((1) << 5))
>>>> [  227.188037] kvm_sys_reg_set_user 3427:(((3) << 19) | ((0) << 16) |
>>>> ((9) << 12) | ((14) << 8) | ((2) << 5))
>>>> [  227.205511] kvm_sys_reg_set_user 3427:SYS_MAIR_EL1
>>>> [  227.218117] kvm_sys_reg_set_user 3427:SYS_PIRE0_EL1
>>> So what is this if not some form of tracing?
>> Ah, i was misunderstood your question. I thought you aksed whether
>> it is related to kernel tracing (which is already existed in kernel)
>> such as tracepoint or debugfs.
> But that's my point: tracepoints such as kvm_sys_access already output
> the name (as well as the encoding), and are likely to be affected by
> this problem.
>
> And that's a much more interesting justification for this change.

Got it. Thanks.

>
> Thanks,
>
> 	M.
>
diff mbox series

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index bd34318..0af5f2f 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -1115,18 +1115,19 @@  static bool access_pmuserenr(struct kvm_vcpu *vcpu, struct sys_reg_params *p,
 	{ SYS_DESC(SYS_DBGWCRn_EL1(n)),					\
 	  trap_wcr, reset_wcr, 0, 0,  get_wcr, set_wcr }
 
-#define PMU_SYS_REG(r)						\
-	SYS_DESC(r), .reset = reset_pmu_reg, .visibility = pmu_visibility
+#define PMU_SYS_REG(name)						\
+	SYS_DESC(SYS_##name), .reset = reset_pmu_reg,			\
+	.visibility = pmu_visibility
 
 /* Macro to expand the PMEVCNTRn_EL0 register */
 #define PMU_PMEVCNTR_EL0(n)						\
-	{ PMU_SYS_REG(SYS_PMEVCNTRn_EL0(n)),				\
+	{ PMU_SYS_REG(PMEVCNTRn_EL0(n)),				\
 	  .reset = reset_pmevcntr, .get_user = get_pmu_evcntr,		\
 	  .access = access_pmu_evcntr, .reg = (PMEVCNTR0_EL0 + n), }
 
 /* Macro to expand the PMEVTYPERn_EL0 register */
 #define PMU_PMEVTYPER_EL0(n)						\
-	{ PMU_SYS_REG(SYS_PMEVTYPERn_EL0(n)),				\
+	{ PMU_SYS_REG(PMEVTYPERn_EL0(n)),				\
 	  .reset = reset_pmevtyper,					\
 	  .access = access_pmu_evtyper, .reg = (PMEVTYPER0_EL0 + n), }
 
@@ -2115,9 +2116,9 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_PMBSR_EL1), undef_access },
 	/* PMBIDR_EL1 is not trapped */
 
-	{ PMU_SYS_REG(SYS_PMINTENSET_EL1),
+	{ PMU_SYS_REG(PMINTENSET_EL1),
 	  .access = access_pminten, .reg = PMINTENSET_EL1 },
-	{ PMU_SYS_REG(SYS_PMINTENCLR_EL1),
+	{ PMU_SYS_REG(PMINTENCLR_EL1),
 	  .access = access_pminten, .reg = PMINTENSET_EL1 },
 	{ SYS_DESC(SYS_PMMIR_EL1), trap_raz_wi },
 
@@ -2164,41 +2165,41 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	{ SYS_DESC(SYS_CTR_EL0), access_ctr },
 	{ SYS_DESC(SYS_SVCR), undef_access },
 
-	{ PMU_SYS_REG(SYS_PMCR_EL0), .access = access_pmcr,
+	{ PMU_SYS_REG(PMCR_EL0), .access = access_pmcr,
 	  .reset = reset_pmcr, .reg = PMCR_EL0 },
-	{ PMU_SYS_REG(SYS_PMCNTENSET_EL0),
+	{ PMU_SYS_REG(PMCNTENSET_EL0),
 	  .access = access_pmcnten, .reg = PMCNTENSET_EL0 },
-	{ PMU_SYS_REG(SYS_PMCNTENCLR_EL0),
+	{ PMU_SYS_REG(PMCNTENCLR_EL0),
 	  .access = access_pmcnten, .reg = PMCNTENSET_EL0 },
-	{ PMU_SYS_REG(SYS_PMOVSCLR_EL0),
+	{ PMU_SYS_REG(PMOVSCLR_EL0),
 	  .access = access_pmovs, .reg = PMOVSSET_EL0 },
 	/*
 	 * PM_SWINC_EL0 is exposed to userspace as RAZ/WI, as it was
 	 * previously (and pointlessly) advertised in the past...
 	 */
-	{ PMU_SYS_REG(SYS_PMSWINC_EL0),
+	{ PMU_SYS_REG(PMSWINC_EL0),
 	  .get_user = get_raz_reg, .set_user = set_wi_reg,
 	  .access = access_pmswinc, .reset = NULL },
-	{ PMU_SYS_REG(SYS_PMSELR_EL0),
+	{ PMU_SYS_REG(PMSELR_EL0),
 	  .access = access_pmselr, .reset = reset_pmselr, .reg = PMSELR_EL0 },
-	{ PMU_SYS_REG(SYS_PMCEID0_EL0),
+	{ PMU_SYS_REG(PMCEID0_EL0),
 	  .access = access_pmceid, .reset = NULL },
-	{ PMU_SYS_REG(SYS_PMCEID1_EL0),
+	{ PMU_SYS_REG(PMCEID1_EL0),
 	  .access = access_pmceid, .reset = NULL },
-	{ PMU_SYS_REG(SYS_PMCCNTR_EL0),
+	{ PMU_SYS_REG(PMCCNTR_EL0),
 	  .access = access_pmu_evcntr, .reset = reset_unknown,
 	  .reg = PMCCNTR_EL0, .get_user = get_pmu_evcntr},
-	{ PMU_SYS_REG(SYS_PMXEVTYPER_EL0),
+	{ PMU_SYS_REG(PMXEVTYPER_EL0),
 	  .access = access_pmu_evtyper, .reset = NULL },
-	{ PMU_SYS_REG(SYS_PMXEVCNTR_EL0),
+	{ PMU_SYS_REG(PMXEVCNTR_EL0),
 	  .access = access_pmu_evcntr, .reset = NULL },
 	/*
 	 * PMUSERENR_EL0 resets as unknown in 64bit mode while it resets as zero
 	 * in 32bit mode. Here we choose to reset it as zero for consistency.
 	 */
-	{ PMU_SYS_REG(SYS_PMUSERENR_EL0), .access = access_pmuserenr,
+	{ PMU_SYS_REG(PMUSERENR_EL0), .access = access_pmuserenr,
 	  .reset = reset_val, .reg = PMUSERENR_EL0, .val = 0 },
-	{ PMU_SYS_REG(SYS_PMOVSSET_EL0),
+	{ PMU_SYS_REG(PMOVSSET_EL0),
 	  .access = access_pmovs, .reg = PMOVSSET_EL0 },
 
 	{ SYS_DESC(SYS_TPIDR_EL0), NULL, reset_unknown, TPIDR_EL0 },
@@ -2354,7 +2355,7 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	 * PMCCFILTR_EL0 resets as unknown in 64bit mode while it resets as zero
 	 * in 32bit mode. Here we choose to reset it as zero for consistency.
 	 */
-	{ PMU_SYS_REG(SYS_PMCCFILTR_EL0), .access = access_pmu_evtyper,
+	{ PMU_SYS_REG(PMCCFILTR_EL0), .access = access_pmu_evtyper,
 	  .reset = reset_val, .reg = PMCCFILTR_EL0, .val = 0 },
 
 	EL2_REG(VPIDR_EL2, access_rw, reset_unknown, 0),