[kvm-unit-tests PATCH 0/3] arm64: minor cleanups for timer test

[kvm-unit-tests PATCH 0/3] arm64: minor cleanups for timer test

[Zenghui Yu]

Hi Drew,

There's some minor cleanups which based on your arm/queue branch for
the timer test.  Please consider taking them if they make the code
a bit better :)

Thanks

Zenghui Yu (3):
  arm/arm64: gic: Move gic_state enumeration to asm/gic.h
  arm64: timer: Use the proper RDist register name in GICv3
  arm64: timer: Use existing helpers to access counter/timers

 arm/timer.c          | 27 ++++++++++-----------------
 lib/arm/asm/gic-v3.h |  4 ++++
 lib/arm/asm/gic.h    |  7 +++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.19.1

[kvm-unit-tests PATCH 0/3] arm64: minor cleanups for timer test

[Zenghui Yu]

Hi Drew,

There's some minor cleanups which based on your arm/queue branch for
the timer test.  Please consider taking them if they make the code
a bit better :)

Thanks

Zenghui Yu (3):
  arm/arm64: gic: Move gic_state enumeration to asm/gic.h
  arm64: timer: Use the proper RDist register name in GICv3
  arm64: timer: Use existing helpers to access counter/timers

 arm/timer.c          | 27 ++++++++++-----------------
 lib/arm/asm/gic-v3.h |  4 ++++
 lib/arm/asm/gic.h    |  7 +++++++
 3 files changed, 21 insertions(+), 17 deletions(-)

-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH 1/3] arm/arm64: gic: Move gic_state enumeration to asm/gic.h

[Zenghui Yu]

The status of each interrupt are defined by the GIC architecture and
maintained by GIC hardware.  They're not specified to the timer HW.
Let's move this software enumeration to a more proper place.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arm/timer.c       | 7 -------
 lib/arm/asm/gic.h | 7 +++++++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index dea364f..94543f2 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -17,13 +17,6 @@
 #define ARCH_TIMER_CTL_IMASK   (1 << 1)
 #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
 
-enum gic_state {
-	GIC_STATE_INACTIVE,
-	GIC_STATE_PENDING,
-	GIC_STATE_ACTIVE,
-	GIC_STATE_ACTIVE_PENDING,
-};
-
 static void *gic_isactiver;
 static void *gic_ispendr;
 static void *gic_isenabler;
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 09826fd..a72e0cd 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -47,6 +47,13 @@
 #ifndef __ASSEMBLY__
 #include <asm/cpumask.h>
 
+enum gic_state {
+	GIC_STATE_INACTIVE,
+	GIC_STATE_PENDING,
+	GIC_STATE_ACTIVE,
+	GIC_STATE_ACTIVE_PENDING,
+};
+
 /*
  * gic_init will try to find all known gics, and then
  * initialize the gic data for the one found.
-- 
2.19.1

[kvm-unit-tests PATCH 1/3] arm/arm64: gic: Move gic_state enumeration to asm/gic.h

[Zenghui Yu]

The status of each interrupt are defined by the GIC architecture and
maintained by GIC hardware.  They're not specified to the timer HW.
Let's move this software enumeration to a more proper place.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arm/timer.c       | 7 -------
 lib/arm/asm/gic.h | 7 +++++++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index dea364f..94543f2 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -17,13 +17,6 @@
 #define ARCH_TIMER_CTL_IMASK   (1 << 1)
 #define ARCH_TIMER_CTL_ISTATUS (1 << 2)
 
-enum gic_state {
-	GIC_STATE_INACTIVE,
-	GIC_STATE_PENDING,
-	GIC_STATE_ACTIVE,
-	GIC_STATE_ACTIVE_PENDING,
-};
-
 static void *gic_isactiver;
 static void *gic_ispendr;
 static void *gic_isenabler;
diff --git a/lib/arm/asm/gic.h b/lib/arm/asm/gic.h
index 09826fd..a72e0cd 100644
--- a/lib/arm/asm/gic.h
+++ b/lib/arm/asm/gic.h
@@ -47,6 +47,13 @@
 #ifndef __ASSEMBLY__
 #include <asm/cpumask.h>
 
+enum gic_state {
+	GIC_STATE_INACTIVE,
+	GIC_STATE_PENDING,
+	GIC_STATE_ACTIVE,
+	GIC_STATE_ACTIVE_PENDING,
+};
+
 /*
  * gic_init will try to find all known gics, and then
  * initialize the gic data for the one found.
-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH 2/3] arm64: timer: Use the proper RDist register name in GICv3

[Zenghui Yu]

We're actually going to read GICR_ISACTIVER0 and GICR_ISPENDR0 (in
SGI_base frame of the redistribitor) to get the active/pending state
of the timer interrupt.  Fix this typo.

And since they have the same value, there's no functional change.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arm/timer.c          | 4 ++--
 lib/arm/asm/gic-v3.h | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 94543f2..10a88f3 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -351,8 +351,8 @@ static void test_init(void)
 		gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
 		break;
 	case 3:
-		gic_isactiver = gicv3_sgi_base() + GICD_ISACTIVER;
-		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
+		gic_isactiver = gicv3_sgi_base() + GICR_ISACTIVER0;
+		gic_ispendr = gicv3_sgi_base() + GICR_ISPENDR0;
 		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
 		gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
 		break;
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 0dc838b..e2736a1 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -32,6 +32,10 @@
 #define GICR_IGROUPR0			GICD_IGROUPR
 #define GICR_ISENABLER0			GICD_ISENABLER
 #define GICR_ICENABLER0			GICD_ICENABLER
+#define GICR_ISPENDR0			GICD_ISPENDR
+#define GICR_ICPENDR0			GICD_ICPENDR
+#define GICR_ISACTIVER0			GICD_ISACTIVER
+#define GICR_ICACTIVER0			GICD_ICACTIVER
 #define GICR_IPRIORITYR0		GICD_IPRIORITYR
 
 #define ICC_SGI1R_AFFINITY_1_SHIFT	16
-- 
2.19.1

[kvm-unit-tests PATCH 2/3] arm64: timer: Use the proper RDist register name in GICv3

[Zenghui Yu]

We're actually going to read GICR_ISACTIVER0 and GICR_ISPENDR0 (in
SGI_base frame of the redistribitor) to get the active/pending state
of the timer interrupt.  Fix this typo.

And since they have the same value, there's no functional change.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arm/timer.c          | 4 ++--
 lib/arm/asm/gic-v3.h | 4 ++++
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 94543f2..10a88f3 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -351,8 +351,8 @@ static void test_init(void)
 		gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
 		break;
 	case 3:
-		gic_isactiver = gicv3_sgi_base() + GICD_ISACTIVER;
-		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
+		gic_isactiver = gicv3_sgi_base() + GICR_ISACTIVER0;
+		gic_ispendr = gicv3_sgi_base() + GICR_ISPENDR0;
 		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
 		gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
 		break;
diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 0dc838b..e2736a1 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -32,6 +32,10 @@
 #define GICR_IGROUPR0			GICD_IGROUPR
 #define GICR_ISENABLER0			GICD_ISENABLER
 #define GICR_ICENABLER0			GICD_ICENABLER
+#define GICR_ISPENDR0			GICD_ISPENDR
+#define GICR_ICPENDR0			GICD_ICPENDR
+#define GICR_ISACTIVER0			GICD_ISACTIVER
+#define GICR_ICACTIVER0			GICD_ICACTIVER
 #define GICR_IPRIORITYR0		GICD_IPRIORITYR
 
 #define ICC_SGI1R_AFFINITY_1_SHIFT	16
-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

[kvm-unit-tests PATCH 3/3] arm64: timer: Use existing helpers to access counter/timers

[Zenghui Yu]

We already have some good helpers to access the counter and timer
registers.  Use them to avoid open coding the accessors again.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arm/timer.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 10a88f3..f5cf775 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -331,7 +331,7 @@ static void test_init(void)
 	vtimer_info.irq_flags = fdt32_to_cpu(data[8]);
 
 	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
-	read_sysreg(cntp_ctl_el0);
+	ptimer_info.read_ctl();
 	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, NULL);
 
 	if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
@@ -366,15 +366,15 @@ static void print_timer_info(void)
 {
 	printf("CNTFRQ_EL0   : 0x%016lx\n", read_sysreg(cntfrq_el0));
 
-	if (!ptimer_unsupported){
-		printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
-		printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
-		printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
+	if (!ptimer_unsupported) {
+		printf("CNTPCT_EL0   : 0x%016lx\n", ptimer_info.read_counter());
+		printf("CNTP_CTL_EL0 : 0x%016lx\n", ptimer_info.read_ctl());
+		printf("CNTP_CVAL_EL0: 0x%016lx\n", ptimer_info.read_cval());
 	}
 
-	printf("CNTVCT_EL0   : 0x%016lx\n", read_sysreg(cntvct_el0));
-	printf("CNTV_CTL_EL0 : 0x%016lx\n", read_sysreg(cntv_ctl_el0));
-	printf("CNTV_CVAL_EL0: 0x%016lx\n", read_sysreg(cntv_cval_el0));
+	printf("CNTVCT_EL0   : 0x%016lx\n", vtimer_info.read_counter());
+	printf("CNTV_CTL_EL0 : 0x%016lx\n", vtimer_info.read_ctl());
+	printf("CNTV_CVAL_EL0: 0x%016lx\n", vtimer_info.read_cval());
 }
 
 int main(int argc, char **argv)
-- 
2.19.1

[kvm-unit-tests PATCH 3/3] arm64: timer: Use existing helpers to access counter/timers

[Zenghui Yu]

We already have some good helpers to access the counter and timer
registers.  Use them to avoid open coding the accessors again.

Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---
 arm/timer.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/arm/timer.c b/arm/timer.c
index 10a88f3..f5cf775 100644
--- a/arm/timer.c
+++ b/arm/timer.c
@@ -331,7 +331,7 @@ static void test_init(void)
 	vtimer_info.irq_flags = fdt32_to_cpu(data[8]);
 
 	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, ptimer_unsupported_handler);
-	read_sysreg(cntp_ctl_el0);
+	ptimer_info.read_ctl();
 	install_exception_handler(EL1H_SYNC, ESR_EL1_EC_UNKNOWN, NULL);
 
 	if (ptimer_unsupported && !ERRATA(7b6b46311a85)) {
@@ -366,15 +366,15 @@ static void print_timer_info(void)
 {
 	printf("CNTFRQ_EL0   : 0x%016lx\n", read_sysreg(cntfrq_el0));
 
-	if (!ptimer_unsupported){
-		printf("CNTPCT_EL0   : 0x%016lx\n", read_sysreg(cntpct_el0));
-		printf("CNTP_CTL_EL0 : 0x%016lx\n", read_sysreg(cntp_ctl_el0));
-		printf("CNTP_CVAL_EL0: 0x%016lx\n", read_sysreg(cntp_cval_el0));
+	if (!ptimer_unsupported) {
+		printf("CNTPCT_EL0   : 0x%016lx\n", ptimer_info.read_counter());
+		printf("CNTP_CTL_EL0 : 0x%016lx\n", ptimer_info.read_ctl());
+		printf("CNTP_CVAL_EL0: 0x%016lx\n", ptimer_info.read_cval());
 	}
 
-	printf("CNTVCT_EL0   : 0x%016lx\n", read_sysreg(cntvct_el0));
-	printf("CNTV_CTL_EL0 : 0x%016lx\n", read_sysreg(cntv_ctl_el0));
-	printf("CNTV_CVAL_EL0: 0x%016lx\n", read_sysreg(cntv_cval_el0));
+	printf("CNTVCT_EL0   : 0x%016lx\n", vtimer_info.read_counter());
+	printf("CNTV_CTL_EL0 : 0x%016lx\n", vtimer_info.read_ctl());
+	printf("CNTV_CVAL_EL0: 0x%016lx\n", vtimer_info.read_cval());
 }
 
 int main(int argc, char **argv)
-- 
2.19.1


_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH 2/3] arm64: timer: Use the proper RDist register name in GICv3

[Alexandru Elisei]

Hi,

On 2/11/20 8:39 AM, Zenghui Yu wrote:
> We're actually going to read GICR_ISACTIVER0 and GICR_ISPENDR0 (in
> SGI_base frame of the redistribitor) to get the active/pending state
> of the timer interrupt.  Fix this typo.
>
> And since they have the same value, there's no functional change.
>
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  arm/timer.c          | 4 ++--
>  lib/arm/asm/gic-v3.h | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index 94543f2..10a88f3 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -351,8 +351,8 @@ static void test_init(void)
>  		gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
>  		break;
>  	case 3:
> -		gic_isactiver = gicv3_sgi_base() + GICD_ISACTIVER;
> -		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
> +		gic_isactiver = gicv3_sgi_base() + GICR_ISACTIVER0;
> +		gic_ispendr = gicv3_sgi_base() + GICR_ISPENDR0;
>  		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
>  		gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
>  		break;
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 0dc838b..e2736a1 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -32,6 +32,10 @@
>  #define GICR_IGROUPR0			GICD_IGROUPR
>  #define GICR_ISENABLER0			GICD_ISENABLER
>  #define GICR_ICENABLER0			GICD_ICENABLER
> +#define GICR_ISPENDR0			GICD_ISPENDR
> +#define GICR_ICPENDR0			GICD_ICPENDR
> +#define GICR_ISACTIVER0			GICD_ISACTIVER
> +#define GICR_ICACTIVER0			GICD_ICACTIVER
>  #define GICR_IPRIORITYR0		GICD_IPRIORITYR
>  
>  #define ICC_SGI1R_AFFINITY_1_SHIFT	16

Looks like an improvement to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

Re: [kvm-unit-tests PATCH 2/3] arm64: timer: Use the proper RDist register name in GICv3

[Alexandru Elisei]

Hi,

On 2/11/20 8:39 AM, Zenghui Yu wrote:
> We're actually going to read GICR_ISACTIVER0 and GICR_ISPENDR0 (in
> SGI_base frame of the redistribitor) to get the active/pending state
> of the timer interrupt.  Fix this typo.
>
> And since they have the same value, there's no functional change.
>
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
>  arm/timer.c          | 4 ++--
>  lib/arm/asm/gic-v3.h | 4 ++++
>  2 files changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/arm/timer.c b/arm/timer.c
> index 94543f2..10a88f3 100644
> --- a/arm/timer.c
> +++ b/arm/timer.c
> @@ -351,8 +351,8 @@ static void test_init(void)
>  		gic_icenabler = gicv2_dist_base() + GICD_ICENABLER;
>  		break;
>  	case 3:
> -		gic_isactiver = gicv3_sgi_base() + GICD_ISACTIVER;
> -		gic_ispendr = gicv3_sgi_base() + GICD_ISPENDR;
> +		gic_isactiver = gicv3_sgi_base() + GICR_ISACTIVER0;
> +		gic_ispendr = gicv3_sgi_base() + GICR_ISPENDR0;
>  		gic_isenabler = gicv3_sgi_base() + GICR_ISENABLER0;
>  		gic_icenabler = gicv3_sgi_base() + GICR_ICENABLER0;
>  		break;
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 0dc838b..e2736a1 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -32,6 +32,10 @@
>  #define GICR_IGROUPR0			GICD_IGROUPR
>  #define GICR_ISENABLER0			GICD_ISENABLER
>  #define GICR_ICENABLER0			GICD_ICENABLER
> +#define GICR_ISPENDR0			GICD_ISPENDR
> +#define GICR_ICPENDR0			GICD_ICPENDR
> +#define GICR_ISACTIVER0			GICD_ISACTIVER
> +#define GICR_ICACTIVER0			GICD_ICACTIVER
>  #define GICR_IPRIORITYR0		GICD_IPRIORITYR
>  
>  #define ICC_SGI1R_AFFINITY_1_SHIFT	16

Looks like an improvement to me:

Reviewed-by: Alexandru Elisei <alexandru.elisei@arm.com>

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [kvm-unit-tests PATCH 0/3] arm64: minor cleanups for timer test

[Andrew Jones]

On Tue, Feb 11, 2020 at 04:38:58PM +0800, Zenghui Yu wrote:
> Hi Drew,
> 
> There's some minor cleanups which based on your arm/queue branch for
> the timer test.  Please consider taking them if they make the code
> a bit better :)
> 
> Thanks
> 
> Zenghui Yu (3):
>   arm/arm64: gic: Move gic_state enumeration to asm/gic.h
>   arm64: timer: Use the proper RDist register name in GICv3
>   arm64: timer: Use existing helpers to access counter/timers
> 
>  arm/timer.c          | 27 ++++++++++-----------------
>  lib/arm/asm/gic-v3.h |  4 ++++
>  lib/arm/asm/gic.h    |  7 +++++++
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 
> -- 
> 2.19.1
> 
>

Applied, thanks


Also, I noticed that the timer tests now take over 8 seconds to run.
I have a patch that speeds that up that I'll send for review in
just a second.

drew 

Re: [kvm-unit-tests PATCH 0/3] arm64: minor cleanups for timer test

[Andrew Jones]

On Tue, Feb 11, 2020 at 04:38:58PM +0800, Zenghui Yu wrote:
> Hi Drew,
> 
> There's some minor cleanups which based on your arm/queue branch for
> the timer test.  Please consider taking them if they make the code
> a bit better :)
> 
> Thanks
> 
> Zenghui Yu (3):
>   arm/arm64: gic: Move gic_state enumeration to asm/gic.h
>   arm64: timer: Use the proper RDist register name in GICv3
>   arm64: timer: Use existing helpers to access counter/timers
> 
>  arm/timer.c          | 27 ++++++++++-----------------
>  lib/arm/asm/gic-v3.h |  4 ++++
>  lib/arm/asm/gic.h    |  7 +++++++
>  3 files changed, 21 insertions(+), 17 deletions(-)
> 
> -- 
> 2.19.1
> 
>

Applied, thanks


Also, I noticed that the timer tests now take over 8 seconds to run.
I have a patch that speeds that up that I'll send for review in
just a second.

drew 

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm