diff mbox

[v4,08/21] KVM: ARM64: Add reset and access handlers for PMXEVTYPER register

Message ID 1446186123-11548-9-git-send-email-zhaoshenglong@huawei.com (mailing list archive)
State New, archived
Headers show

Commit Message

Shannon Zhao Oct. 30, 2015, 6:21 a.m. UTC 
From: Shannon Zhao <shannon.zhao@linaro.org>

Since the reset value of PMXEVTYPER is UNKNOWN, use reset_unknown or
reset_unknown_cp15 for its reset handler. Add access handler which
emulates writing and reading PMXEVTYPER register. When writing to
PMXEVTYPER, call kvm_pmu_set_counter_event_type to create a perf_event
for the selected event type.

Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
---
 arch/arm64/kvm/sys_regs.c | 26 ++++++++++++++++++++++++--
 1 file changed, 24 insertions(+), 2 deletions(-)

Comments

Christopher Covington Nov. 2, 2015, 8:54 p.m. UTC | #1 
Hi Shannon,

On 10/30/2015 02:21 AM, Shannon Zhao wrote:
> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Since the reset value of PMXEVTYPER is UNKNOWN, use reset_unknown or
> reset_unknown_cp15 for its reset handler. Add access handler which
> emulates writing and reading PMXEVTYPER register. When writing to
> PMXEVTYPER, call kvm_pmu_set_counter_event_type to create a perf_event
> for the selected event type.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index cb82b15..4e606ea 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -491,6 +491,17 @@ static bool access_pmu_regs(struct kvm_vcpu *vcpu,
>  
>  	if (p->is_write) {
>  		switch (r->reg) {
> +		case PMXEVTYPER_EL0: {
> +			val = vcpu_sys_reg(vcpu, PMSELR_EL0);
> +			kvm_pmu_set_counter_event_type(vcpu,
> +						       *vcpu_reg(vcpu, p->Rt),
> +						       val);
> +			vcpu_sys_reg(vcpu, PMXEVTYPER_EL0) =
> +							 *vcpu_reg(vcpu, p->Rt);

Why does PMXEVTYPER get set directly? It seems like it could have an accessor
that redirected to PMEVTYPER<n>.

> +			vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + val) =
> +							 *vcpu_reg(vcpu, p->Rt);

I tried to look around briefly but couldn't find counter number range checking
in the PMSELR handler or here. Should there be some here and in PMXEVCNTR?

Thanks,
Christopher Covington
Shannon Zhao Nov. 3, 2015, 2:41 a.m. UTC | #2 
On 2015/11/3 4:54, Christopher Covington wrote:
> Hi Shannon,
> 
> On 10/30/2015 02:21 AM, Shannon Zhao wrote:
>> From: Shannon Zhao <shannon.zhao@linaro.org>
>>
>> Since the reset value of PMXEVTYPER is UNKNOWN, use reset_unknown or
>> reset_unknown_cp15 for its reset handler. Add access handler which
>> emulates writing and reading PMXEVTYPER register. When writing to
>> PMXEVTYPER, call kvm_pmu_set_counter_event_type to create a perf_event
>> for the selected event type.
>>
>> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> ---
>>  arch/arm64/kvm/sys_regs.c | 26 ++++++++++++++++++++++++--
>>  1 file changed, 24 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> index cb82b15..4e606ea 100644
>> --- a/arch/arm64/kvm/sys_regs.c
>> +++ b/arch/arm64/kvm/sys_regs.c
>> @@ -491,6 +491,17 @@ static bool access_pmu_regs(struct kvm_vcpu *vcpu,
>>  
>>  	if (p->is_write) {
>>  		switch (r->reg) {
>> +		case PMXEVTYPER_EL0: {
>> +			val = vcpu_sys_reg(vcpu, PMSELR_EL0);
>> +			kvm_pmu_set_counter_event_type(vcpu,
>> +						       *vcpu_reg(vcpu, p->Rt),
>> +						       val);
>> +			vcpu_sys_reg(vcpu, PMXEVTYPER_EL0) =
>> +							 *vcpu_reg(vcpu, p->Rt);
> 
> Why does PMXEVTYPER get set directly? It seems like it could have an accessor
> that redirected to PMEVTYPER<n>.
> 
Yeah, that's what this patch does. It gets the counter index from
PMSELR_EL0 register, then set the event type, create perf_event, store
event type to PMEVTYPER<n>, etc.

>> +			vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + val) =
>> +							 *vcpu_reg(vcpu, p->Rt);
> 
> I tried to look around briefly but couldn't find counter number range checking
> in the PMSELR handler or here. Should there be some here and in PMXEVCNTR?
> 
Ok, will fix this. Thanks.
Marc Zyngier Nov. 30, 2015, 6:12 p.m. UTC | #3 
On Fri, 30 Oct 2015 14:21:50 +0800
Shannon Zhao <zhaoshenglong@huawei.com> wrote:

> From: Shannon Zhao <shannon.zhao@linaro.org>
> 
> Since the reset value of PMXEVTYPER is UNKNOWN, use reset_unknown or
> reset_unknown_cp15 for its reset handler. Add access handler which
> emulates writing and reading PMXEVTYPER register. When writing to
> PMXEVTYPER, call kvm_pmu_set_counter_event_type to create a perf_event
> for the selected event type.
> 
> Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
> ---
>  arch/arm64/kvm/sys_regs.c | 26 ++++++++++++++++++++++++--
>  1 file changed, 24 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
> index cb82b15..4e606ea 100644
> --- a/arch/arm64/kvm/sys_regs.c
> +++ b/arch/arm64/kvm/sys_regs.c
> @@ -491,6 +491,17 @@ static bool access_pmu_regs(struct kvm_vcpu *vcpu,
>  
>  	if (p->is_write) {
>  		switch (r->reg) {
> +		case PMXEVTYPER_EL0: {
> +			val = vcpu_sys_reg(vcpu, PMSELR_EL0);
> +			kvm_pmu_set_counter_event_type(vcpu,
> +						       *vcpu_reg(vcpu, p->Rt),
> +						       val);

You are blindingly truncating 64bit values to u32. Is that intentional?

> +			vcpu_sys_reg(vcpu, PMXEVTYPER_EL0) =
> +							 *vcpu_reg(vcpu, p->Rt);
> +			vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + val) =
> +							 *vcpu_reg(vcpu, p->Rt);

Please do not break assignments like this, it makes the code
unreadable. I don't care what the 80-character police says... ;-)

> +			break;
> +		}
>  		case PMCR_EL0: {
>  			/* Only update writeable bits of PMCR */
>  			val = vcpu_sys_reg(vcpu, r->reg);
> @@ -735,7 +746,7 @@ static const struct sys_reg_desc sys_reg_descs[] = {
>  	  trap_raz_wi },
>  	/* PMXEVTYPER_EL0 */
>  	{ Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1101), Op2(0b001),
> -	  trap_raz_wi },
> +	  access_pmu_regs, reset_unknown, PMXEVTYPER_EL0 },
>  	/* PMXEVCNTR_EL0 */
>  	{ Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1101), Op2(0b010),
>  	  trap_raz_wi },
> @@ -951,6 +962,16 @@ static bool access_pmu_cp15_regs(struct kvm_vcpu *vcpu,
>  
>  	if (p->is_write) {
>  		switch (r->reg) {
> +		case c9_PMXEVTYPER: {
> +			val = vcpu_cp15(vcpu, c9_PMSELR);
> +			kvm_pmu_set_counter_event_type(vcpu,
> +						       *vcpu_reg(vcpu, p->Rt),
> +						       val);
> +			vcpu_cp15(vcpu, c9_PMXEVTYPER) = *vcpu_reg(vcpu, p->Rt);
> +			vcpu_cp15(vcpu, c14_PMEVTYPER0 + val) =
> +							 *vcpu_reg(vcpu, p->Rt);
> +			break;
> +		}
>  		case c9_PMCR: {
>  			/* Only update writeable bits of PMCR */
>  			val = vcpu_cp15(vcpu, r->reg);
> @@ -1024,7 +1045,8 @@ static const struct sys_reg_desc cp15_regs[] = {
>  	{ Op1( 0), CRn( 9), CRm(12), Op2( 7), access_pmu_cp15_regs,
>  	  reset_pmceid, c9_PMCEID1 },
>  	{ Op1( 0), CRn( 9), CRm(13), Op2( 0), trap_raz_wi },
> -	{ Op1( 0), CRn( 9), CRm(13), Op2( 1), trap_raz_wi },
> +	{ Op1( 0), CRn( 9), CRm(13), Op2( 1), access_pmu_cp15_regs,
> +	  reset_unknown_cp15, c9_PMXEVTYPER },
>  	{ Op1( 0), CRn( 9), CRm(13), Op2( 2), trap_raz_wi },
>  	{ Op1( 0), CRn( 9), CRm(14), Op2( 0), trap_raz_wi },
>  	{ Op1( 0), CRn( 9), CRm(14), Op2( 1), trap_raz_wi },

Thanks,

	M.
Shannon Zhao Dec. 1, 2015, 2:42 a.m. UTC | #4 
On 2015/12/1 2:12, Marc Zyngier wrote:
> On Fri, 30 Oct 2015 14:21:50 +0800
> Shannon Zhao <zhaoshenglong@huawei.com> wrote:
> 
>> > From: Shannon Zhao <shannon.zhao@linaro.org>
>> > 
>> > Since the reset value of PMXEVTYPER is UNKNOWN, use reset_unknown or
>> > reset_unknown_cp15 for its reset handler. Add access handler which
>> > emulates writing and reading PMXEVTYPER register. When writing to
>> > PMXEVTYPER, call kvm_pmu_set_counter_event_type to create a perf_event
>> > for the selected event type.
>> > 
>> > Signed-off-by: Shannon Zhao <shannon.zhao@linaro.org>
>> > ---
>> >  arch/arm64/kvm/sys_regs.c | 26 ++++++++++++++++++++++++--
>> >  1 file changed, 24 insertions(+), 2 deletions(-)
>> > 
>> > diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
>> > index cb82b15..4e606ea 100644
>> > --- a/arch/arm64/kvm/sys_regs.c
>> > +++ b/arch/arm64/kvm/sys_regs.c
>> > @@ -491,6 +491,17 @@ static bool access_pmu_regs(struct kvm_vcpu *vcpu,
>> >  
>> >  	if (p->is_write) {
>> >  		switch (r->reg) {
>> > +		case PMXEVTYPER_EL0: {
>> > +			val = vcpu_sys_reg(vcpu, PMSELR_EL0);
>> > +			kvm_pmu_set_counter_event_type(vcpu,
>> > +						       *vcpu_reg(vcpu, p->Rt),
>> > +						       val);
> You are blindingly truncating 64bit values to u32. Is that intentional?
> 
Yeah, the register PMXEVTYPER_EL0 and PMSELR_EL0 are all 32bit.
diff mbox

Patch

diff --git a/arch/arm64/kvm/sys_regs.c b/arch/arm64/kvm/sys_regs.c
index cb82b15..4e606ea 100644
--- a/arch/arm64/kvm/sys_regs.c
+++ b/arch/arm64/kvm/sys_regs.c
@@ -491,6 +491,17 @@  static bool access_pmu_regs(struct kvm_vcpu *vcpu,
 
 	if (p->is_write) {
 		switch (r->reg) {
+		case PMXEVTYPER_EL0: {
+			val = vcpu_sys_reg(vcpu, PMSELR_EL0);
+			kvm_pmu_set_counter_event_type(vcpu,
+						       *vcpu_reg(vcpu, p->Rt),
+						       val);
+			vcpu_sys_reg(vcpu, PMXEVTYPER_EL0) =
+							 *vcpu_reg(vcpu, p->Rt);
+			vcpu_sys_reg(vcpu, PMEVTYPER0_EL0 + val) =
+							 *vcpu_reg(vcpu, p->Rt);
+			break;
+		}
 		case PMCR_EL0: {
 			/* Only update writeable bits of PMCR */
 			val = vcpu_sys_reg(vcpu, r->reg);
@@ -735,7 +746,7 @@  static const struct sys_reg_desc sys_reg_descs[] = {
 	  trap_raz_wi },
 	/* PMXEVTYPER_EL0 */
 	{ Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1101), Op2(0b001),
-	  trap_raz_wi },
+	  access_pmu_regs, reset_unknown, PMXEVTYPER_EL0 },
 	/* PMXEVCNTR_EL0 */
 	{ Op0(0b11), Op1(0b011), CRn(0b1001), CRm(0b1101), Op2(0b010),
 	  trap_raz_wi },
@@ -951,6 +962,16 @@  static bool access_pmu_cp15_regs(struct kvm_vcpu *vcpu,
 
 	if (p->is_write) {
 		switch (r->reg) {
+		case c9_PMXEVTYPER: {
+			val = vcpu_cp15(vcpu, c9_PMSELR);
+			kvm_pmu_set_counter_event_type(vcpu,
+						       *vcpu_reg(vcpu, p->Rt),
+						       val);
+			vcpu_cp15(vcpu, c9_PMXEVTYPER) = *vcpu_reg(vcpu, p->Rt);
+			vcpu_cp15(vcpu, c14_PMEVTYPER0 + val) =
+							 *vcpu_reg(vcpu, p->Rt);
+			break;
+		}
 		case c9_PMCR: {
 			/* Only update writeable bits of PMCR */
 			val = vcpu_cp15(vcpu, r->reg);
@@ -1024,7 +1045,8 @@  static const struct sys_reg_desc cp15_regs[] = {
 	{ Op1( 0), CRn( 9), CRm(12), Op2( 7), access_pmu_cp15_regs,
 	  reset_pmceid, c9_PMCEID1 },
 	{ Op1( 0), CRn( 9), CRm(13), Op2( 0), trap_raz_wi },
-	{ Op1( 0), CRn( 9), CRm(13), Op2( 1), trap_raz_wi },
+	{ Op1( 0), CRn( 9), CRm(13), Op2( 1), access_pmu_cp15_regs,
+	  reset_unknown_cp15, c9_PMXEVTYPER },
 	{ Op1( 0), CRn( 9), CRm(13), Op2( 2), trap_raz_wi },
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 0), trap_raz_wi },
 	{ Op1( 0), CRn( 9), CRm(14), Op2( 1), trap_raz_wi },