[PATCH kvm-unit-tests v2] arm/arm64: gicv3: support up to 8 redistributor regions regions

[PATCH kvm-unit-tests v2] arm/arm64: gicv3: support up to 8 redistributor regions regions

[Andrew Jones]

We need to support at least two redistributor regions in order to
support more than 123 vcpus (we select 8 because that should be
plenty). Also bump NR_CPUS to 512, since that's what KVM currently
supports.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
v2:
 - Peter asked if it would be neater to be more general. At first
   I interpreted that as better, i.e. more general, but immediately
   after starting the approach I saw how much _neater_ the code is.
   So this version is indeed neater code and better (more general).
   Thanks Peter!


 lib/arm/asm/gic-v3.h |  3 +++
 lib/arm/asm/setup.h  |  2 +-
 lib/arm/gic-v3.c     | 23 +++++++++++++----------
 lib/arm/gic.c        | 14 +++++++++-----
 4 files changed, 26 insertions(+), 16 deletions(-)

diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
index 1dceb9541f62..347be2f9da17 100644
--- a/lib/arm/asm/gic-v3.h
+++ b/lib/arm/asm/gic-v3.h
@@ -49,8 +49,11 @@
 #include <asm/smp.h>
 #include <asm/io.h>
 
+#define GICV3_NR_REDISTS 8
+
 struct gicv3_data {
 	void *dist_base;
+	void *redist_bases[GICV3_NR_REDISTS];
 	void *redist_base[NR_CPUS];
 	unsigned int irq_nr;
 };
diff --git a/lib/arm/asm/setup.h b/lib/arm/asm/setup.h
index b57ea13b9dd2..3215814603e4 100644
--- a/lib/arm/asm/setup.h
+++ b/lib/arm/asm/setup.h
@@ -9,7 +9,7 @@
 #include <asm/page.h>
 #include <asm/pgtable-hwdef.h>
 
-#define NR_CPUS			255
+#define NR_CPUS			512
 extern u64 cpus[NR_CPUS];	/* per-cpu IDs (MPIDRs) */
 extern int nr_cpus;
 
diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
index 9b3fa5730478..7f41e0a1da01 100644
--- a/lib/arm/gic-v3.c
+++ b/lib/arm/gic-v3.c
@@ -9,17 +9,20 @@
 void gicv3_set_redist_base(size_t stride)
 {
 	u32 aff = mpidr_compress(get_mpidr());
-	void *ptr = gicv3_data.redist_base[0];
 	u64 typer;
-
-	do {
-		typer = gicv3_read_typer(ptr + GICR_TYPER);
-		if ((typer >> 32) == aff) {
-			gicv3_redist_base() = ptr;
-			return;
-		}
-		ptr += stride; /* skip RD_base, SGI_base, etc. */
-	} while (!(typer & GICR_TYPER_LAST));
+	int i = 0;
+
+	while (gicv3_data.redist_bases[i]) {
+		void *ptr = gicv3_data.redist_bases[i];
+		do {
+			typer = gicv3_read_typer(ptr + GICR_TYPER);
+			if ((typer >> 32) == aff) {
+				gicv3_redist_base() = ptr;
+				return;
+			}
+			ptr += stride; /* skip RD_base, SGI_base, etc. */
+		} while (!(typer & GICR_TYPER_LAST));
+	}
 
 	/* should never reach here */
 	assert(0);
diff --git a/lib/arm/gic.c b/lib/arm/gic.c
index 59273b1716d6..94301169215c 100644
--- a/lib/arm/gic.c
+++ b/lib/arm/gic.c
@@ -49,7 +49,7 @@ gic_get_dt_bases(const char *compatible, void **base1, void **base2)
 	struct dt_pbus_reg reg;
 	struct dt_device gic;
 	struct dt_bus bus;
-	int node, ret;
+	int node, ret, i;
 
 	dt_bus_init_defaults(&bus);
 	dt_device_init(&gic, &bus, NULL);
@@ -66,9 +66,13 @@ gic_get_dt_bases(const char *compatible, void **base1, void **base2)
 	assert(ret == 0);
 	*base1 = ioremap(reg.addr, reg.size);
 
-	ret = dt_pbus_translate(&gic, 1, &reg);
-	assert(ret == 0);
-	*base2 = ioremap(reg.addr, reg.size);
+	for (i = 0; i < GICV3_NR_REDISTS; ++i) {
+		ret = dt_pbus_translate(&gic, i + 1, &reg);
+		if (ret == -FDT_ERR_NOTFOUND)
+			break;
+		assert(ret == 0);
+		base2[i] = ioremap(reg.addr, reg.size);
+	}
 
 	return true;
 }
@@ -82,7 +86,7 @@ int gicv2_init(void)
 int gicv3_init(void)
 {
 	return gic_get_dt_bases("arm,gic-v3", &gicv3_data.dist_base,
-			&gicv3_data.redist_base[0]);
+			&gicv3_data.redist_bases[0]);
 }
 
 int gic_version(void)
-- 
2.17.1

Re: [PATCH kvm-unit-tests v2] arm/arm64: gicv3: support up to 8 redistributor regions regions

[Christoffer Dall]

On Tue, Sep 04, 2018 at 10:13:36AM +0200, Andrew Jones wrote:
> We need to support at least two redistributor regions in order to
> support more than 123 vcpus (we select 8 because that should be
> plenty). Also bump NR_CPUS to 512, since that's what KVM currently
> supports.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
> v2:
>  - Peter asked if it would be neater to be more general. At first
>    I interpreted that as better, i.e. more general, but immediately
>    after starting the approach I saw how much _neater_ the code is.
>    So this version is indeed neater code and better (more general).
>    Thanks Peter!
> 
> 
>  lib/arm/asm/gic-v3.h |  3 +++
>  lib/arm/asm/setup.h  |  2 +-
>  lib/arm/gic-v3.c     | 23 +++++++++++++----------
>  lib/arm/gic.c        | 14 +++++++++-----
>  4 files changed, 26 insertions(+), 16 deletions(-)
> 
> diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> index 1dceb9541f62..347be2f9da17 100644
> --- a/lib/arm/asm/gic-v3.h
> +++ b/lib/arm/asm/gic-v3.h
> @@ -49,8 +49,11 @@
>  #include <asm/smp.h>
>  #include <asm/io.h>
>  
> +#define GICV3_NR_REDISTS 8
> +
>  struct gicv3_data {
>  	void *dist_base;
> +	void *redist_bases[GICV3_NR_REDISTS];
>  	void *redist_base[NR_CPUS];
>  	unsigned int irq_nr;
>  };
> diff --git a/lib/arm/asm/setup.h b/lib/arm/asm/setup.h
> index b57ea13b9dd2..3215814603e4 100644
> --- a/lib/arm/asm/setup.h
> +++ b/lib/arm/asm/setup.h
> @@ -9,7 +9,7 @@
>  #include <asm/page.h>
>  #include <asm/pgtable-hwdef.h>
>  
> -#define NR_CPUS			255
> +#define NR_CPUS			512
>  extern u64 cpus[NR_CPUS];	/* per-cpu IDs (MPIDRs) */
>  extern int nr_cpus;
>  
> diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> index 9b3fa5730478..7f41e0a1da01 100644
> --- a/lib/arm/gic-v3.c
> +++ b/lib/arm/gic-v3.c
> @@ -9,17 +9,20 @@
>  void gicv3_set_redist_base(size_t stride)
>  {
>  	u32 aff = mpidr_compress(get_mpidr());
> -	void *ptr = gicv3_data.redist_base[0];
>  	u64 typer;
> -
> -	do {
> -		typer = gicv3_read_typer(ptr + GICR_TYPER);
> -		if ((typer >> 32) == aff) {
> -			gicv3_redist_base() = ptr;
> -			return;
> -		}
> -		ptr += stride; /* skip RD_base, SGI_base, etc. */
> -	} while (!(typer & GICR_TYPER_LAST));
> +	int i = 0;
> +
> +	while (gicv3_data.redist_bases[i]) {

I don't get this.  We never change I?

(conincidentally, my test of gicv3-ipi never completes, I wonder if
these concepts are related...)


> +		void *ptr = gicv3_data.redist_bases[i];
> +		do {
> +			typer = gicv3_read_typer(ptr + GICR_TYPER);
> +			if ((typer >> 32) == aff) {
> +				gicv3_redist_base() = ptr;
> +				return;
> +			}
> +			ptr += stride; /* skip RD_base, SGI_base, etc. */
> +		} while (!(typer & GICR_TYPER_LAST));
> +	}
>  
>  	/* should never reach here */
>  	assert(0);
> diff --git a/lib/arm/gic.c b/lib/arm/gic.c
> index 59273b1716d6..94301169215c 100644
> --- a/lib/arm/gic.c
> +++ b/lib/arm/gic.c
> @@ -49,7 +49,7 @@ gic_get_dt_bases(const char *compatible, void **base1, void **base2)
>  	struct dt_pbus_reg reg;
>  	struct dt_device gic;
>  	struct dt_bus bus;
> -	int node, ret;
> +	int node, ret, i;
>  
>  	dt_bus_init_defaults(&bus);
>  	dt_device_init(&gic, &bus, NULL);
> @@ -66,9 +66,13 @@ gic_get_dt_bases(const char *compatible, void **base1, void **base2)
>  	assert(ret == 0);
>  	*base1 = ioremap(reg.addr, reg.size);
>  
> -	ret = dt_pbus_translate(&gic, 1, &reg);
> -	assert(ret == 0);
> -	*base2 = ioremap(reg.addr, reg.size);
> +	for (i = 0; i < GICV3_NR_REDISTS; ++i) {
> +		ret = dt_pbus_translate(&gic, i + 1, &reg);
> +		if (ret == -FDT_ERR_NOTFOUND)
> +			break;
> +		assert(ret == 0);
> +		base2[i] = ioremap(reg.addr, reg.size);
> +	}
>  
>  	return true;
>  }
> @@ -82,7 +86,7 @@ int gicv2_init(void)
>  int gicv3_init(void)
>  {
>  	return gic_get_dt_bases("arm,gic-v3", &gicv3_data.dist_base,
> -			&gicv3_data.redist_base[0]);
> +			&gicv3_data.redist_bases[0]);
>  }
>  
>  int gic_version(void)
> -- 
> 2.17.1
> 

Thanks,

    Christoffer

Re: [PATCH kvm-unit-tests v2] arm/arm64: gicv3: support up to 8 redistributor regions regions

[Andrew Jones]

On Tue, Sep 04, 2018 at 03:29:26PM +0200, Christoffer Dall wrote:
> On Tue, Sep 04, 2018 at 10:13:36AM +0200, Andrew Jones wrote:
> > We need to support at least two redistributor regions in order to
> > support more than 123 vcpus (we select 8 because that should be
> > plenty). Also bump NR_CPUS to 512, since that's what KVM currently
> > supports.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> > v2:
> >  - Peter asked if it would be neater to be more general. At first
> >    I interpreted that as better, i.e. more general, but immediately
> >    after starting the approach I saw how much _neater_ the code is.
> >    So this version is indeed neater code and better (more general).
> >    Thanks Peter!
> > 
> > 
> >  lib/arm/asm/gic-v3.h |  3 +++
> >  lib/arm/asm/setup.h  |  2 +-
> >  lib/arm/gic-v3.c     | 23 +++++++++++++----------
> >  lib/arm/gic.c        | 14 +++++++++-----
> >  4 files changed, 26 insertions(+), 16 deletions(-)
> > 
> > diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> > index 1dceb9541f62..347be2f9da17 100644
> > --- a/lib/arm/asm/gic-v3.h
> > +++ b/lib/arm/asm/gic-v3.h
> > @@ -49,8 +49,11 @@
> >  #include <asm/smp.h>
> >  #include <asm/io.h>
> >  
> > +#define GICV3_NR_REDISTS 8
> > +
> >  struct gicv3_data {
> >  	void *dist_base;
> > +	void *redist_bases[GICV3_NR_REDISTS];
> >  	void *redist_base[NR_CPUS];
> >  	unsigned int irq_nr;
> >  };
> > diff --git a/lib/arm/asm/setup.h b/lib/arm/asm/setup.h
> > index b57ea13b9dd2..3215814603e4 100644
> > --- a/lib/arm/asm/setup.h
> > +++ b/lib/arm/asm/setup.h
> > @@ -9,7 +9,7 @@
> >  #include <asm/page.h>
> >  #include <asm/pgtable-hwdef.h>
> >  
> > -#define NR_CPUS			255
> > +#define NR_CPUS			512
> >  extern u64 cpus[NR_CPUS];	/* per-cpu IDs (MPIDRs) */
> >  extern int nr_cpus;
> >  
> > diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> > index 9b3fa5730478..7f41e0a1da01 100644
> > --- a/lib/arm/gic-v3.c
> > +++ b/lib/arm/gic-v3.c
> > @@ -9,17 +9,20 @@
> >  void gicv3_set_redist_base(size_t stride)
> >  {
> >  	u32 aff = mpidr_compress(get_mpidr());
> > -	void *ptr = gicv3_data.redist_base[0];
> >  	u64 typer;
> > -
> > -	do {
> > -		typer = gicv3_read_typer(ptr + GICR_TYPER);
> > -		if ((typer >> 32) == aff) {
> > -			gicv3_redist_base() = ptr;
> > -			return;
> > -		}
> > -		ptr += stride; /* skip RD_base, SGI_base, etc. */
> > -	} while (!(typer & GICR_TYPER_LAST));
> > +	int i = 0;
> > +
> > +	while (gicv3_data.redist_bases[i]) {
> 
> I don't get this.  We never change I?
> 
> (conincidentally, my test of gicv3-ipi never completes, I wonder if
> these concepts are related...)

Darn it. I dropped the '++i' I had in v1. My regression testing naturally
only needs i=0 (I don't have anything with > 123 cpus to test on), so the
problem didn't show up for me.

Maybe you can add a ++i in and see if it works for you before I send v3?
If it's still busted I'll have to stop shotgunning code, and maybe even
find a way to test it myself.

Thanks,
drew

Re: [PATCH kvm-unit-tests v2] arm/arm64: gicv3: support up to 8 redistributor regions regions

[Christoffer Dall]

On Tue, Sep 04, 2018 at 04:30:12PM +0200, Andrew Jones wrote:
> On Tue, Sep 04, 2018 at 03:29:26PM +0200, Christoffer Dall wrote:
> > On Tue, Sep 04, 2018 at 10:13:36AM +0200, Andrew Jones wrote:
> > > We need to support at least two redistributor regions in order to
> > > support more than 123 vcpus (we select 8 because that should be
> > > plenty). Also bump NR_CPUS to 512, since that's what KVM currently
> > > supports.
> > > 
> > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > ---
> > > v2:
> > >  - Peter asked if it would be neater to be more general. At first
> > >    I interpreted that as better, i.e. more general, but immediately
> > >    after starting the approach I saw how much _neater_ the code is.
> > >    So this version is indeed neater code and better (more general).
> > >    Thanks Peter!
> > > 
> > > 
> > >  lib/arm/asm/gic-v3.h |  3 +++
> > >  lib/arm/asm/setup.h  |  2 +-
> > >  lib/arm/gic-v3.c     | 23 +++++++++++++----------
> > >  lib/arm/gic.c        | 14 +++++++++-----
> > >  4 files changed, 26 insertions(+), 16 deletions(-)
> > > 
> > > diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> > > index 1dceb9541f62..347be2f9da17 100644
> > > --- a/lib/arm/asm/gic-v3.h
> > > +++ b/lib/arm/asm/gic-v3.h
> > > @@ -49,8 +49,11 @@
> > >  #include <asm/smp.h>
> > >  #include <asm/io.h>
> > >  
> > > +#define GICV3_NR_REDISTS 8
> > > +
> > >  struct gicv3_data {
> > >  	void *dist_base;
> > > +	void *redist_bases[GICV3_NR_REDISTS];
> > >  	void *redist_base[NR_CPUS];
> > >  	unsigned int irq_nr;
> > >  };
> > > diff --git a/lib/arm/asm/setup.h b/lib/arm/asm/setup.h
> > > index b57ea13b9dd2..3215814603e4 100644
> > > --- a/lib/arm/asm/setup.h
> > > +++ b/lib/arm/asm/setup.h
> > > @@ -9,7 +9,7 @@
> > >  #include <asm/page.h>
> > >  #include <asm/pgtable-hwdef.h>
> > >  
> > > -#define NR_CPUS			255
> > > +#define NR_CPUS			512
> > >  extern u64 cpus[NR_CPUS];	/* per-cpu IDs (MPIDRs) */
> > >  extern int nr_cpus;
> > >  
> > > diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> > > index 9b3fa5730478..7f41e0a1da01 100644
> > > --- a/lib/arm/gic-v3.c
> > > +++ b/lib/arm/gic-v3.c
> > > @@ -9,17 +9,20 @@
> > >  void gicv3_set_redist_base(size_t stride)
> > >  {
> > >  	u32 aff = mpidr_compress(get_mpidr());
> > > -	void *ptr = gicv3_data.redist_base[0];
> > >  	u64 typer;
> > > -
> > > -	do {
> > > -		typer = gicv3_read_typer(ptr + GICR_TYPER);
> > > -		if ((typer >> 32) == aff) {
> > > -			gicv3_redist_base() = ptr;
> > > -			return;
> > > -		}
> > > -		ptr += stride; /* skip RD_base, SGI_base, etc. */
> > > -	} while (!(typer & GICR_TYPER_LAST));
> > > +	int i = 0;
> > > +
> > > +	while (gicv3_data.redist_bases[i]) {
> > 
> > I don't get this.  We never change I?
> > 
> > (conincidentally, my test of gicv3-ipi never completes, I wonder if
> > these concepts are related...)
> 
> Darn it. I dropped the '++i' I had in v1. My regression testing naturally
> only needs i=0 (I don't have anything with > 123 cpus to test on), so the
> problem didn't show up for me.
> 
> Maybe you can add a ++i in and see if it works for you before I send v3?
> If it's still busted I'll have to stop shotgunning code, and maybe even
> find a way to test it myself.
> 

Indeed, works perfectly.  With an added i++:

Tested-by: Christoffer Dall <christoffer.dall@arm.com>


Thanks,

    Christoffer

Re: [PATCH kvm-unit-tests v2] arm/arm64: gicv3: support up to 8 redistributor regions regions

[Andrew Jones]

On Wed, Sep 05, 2018 at 12:13:36PM +0200, Christoffer Dall wrote:
> On Tue, Sep 04, 2018 at 04:30:12PM +0200, Andrew Jones wrote:
> > On Tue, Sep 04, 2018 at 03:29:26PM +0200, Christoffer Dall wrote:
> > > On Tue, Sep 04, 2018 at 10:13:36AM +0200, Andrew Jones wrote:
> > > > We need to support at least two redistributor regions in order to
> > > > support more than 123 vcpus (we select 8 because that should be
> > > > plenty). Also bump NR_CPUS to 512, since that's what KVM currently
> > > > supports.
> > > > 
> > > > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > > > ---
> > > > v2:
> > > >  - Peter asked if it would be neater to be more general. At first
> > > >    I interpreted that as better, i.e. more general, but immediately
> > > >    after starting the approach I saw how much _neater_ the code is.
> > > >    So this version is indeed neater code and better (more general).
> > > >    Thanks Peter!
> > > > 
> > > > 
> > > >  lib/arm/asm/gic-v3.h |  3 +++
> > > >  lib/arm/asm/setup.h  |  2 +-
> > > >  lib/arm/gic-v3.c     | 23 +++++++++++++----------
> > > >  lib/arm/gic.c        | 14 +++++++++-----
> > > >  4 files changed, 26 insertions(+), 16 deletions(-)
> > > > 
> > > > diff --git a/lib/arm/asm/gic-v3.h b/lib/arm/asm/gic-v3.h
> > > > index 1dceb9541f62..347be2f9da17 100644
> > > > --- a/lib/arm/asm/gic-v3.h
> > > > +++ b/lib/arm/asm/gic-v3.h
> > > > @@ -49,8 +49,11 @@
> > > >  #include <asm/smp.h>
> > > >  #include <asm/io.h>
> > > >  
> > > > +#define GICV3_NR_REDISTS 8
> > > > +
> > > >  struct gicv3_data {
> > > >  	void *dist_base;
> > > > +	void *redist_bases[GICV3_NR_REDISTS];
> > > >  	void *redist_base[NR_CPUS];
> > > >  	unsigned int irq_nr;
> > > >  };
> > > > diff --git a/lib/arm/asm/setup.h b/lib/arm/asm/setup.h
> > > > index b57ea13b9dd2..3215814603e4 100644
> > > > --- a/lib/arm/asm/setup.h
> > > > +++ b/lib/arm/asm/setup.h
> > > > @@ -9,7 +9,7 @@
> > > >  #include <asm/page.h>
> > > >  #include <asm/pgtable-hwdef.h>
> > > >  
> > > > -#define NR_CPUS			255
> > > > +#define NR_CPUS			512
> > > >  extern u64 cpus[NR_CPUS];	/* per-cpu IDs (MPIDRs) */
> > > >  extern int nr_cpus;
> > > >  
> > > > diff --git a/lib/arm/gic-v3.c b/lib/arm/gic-v3.c
> > > > index 9b3fa5730478..7f41e0a1da01 100644
> > > > --- a/lib/arm/gic-v3.c
> > > > +++ b/lib/arm/gic-v3.c
> > > > @@ -9,17 +9,20 @@
> > > >  void gicv3_set_redist_base(size_t stride)
> > > >  {
> > > >  	u32 aff = mpidr_compress(get_mpidr());
> > > > -	void *ptr = gicv3_data.redist_base[0];
> > > >  	u64 typer;
> > > > -
> > > > -	do {
> > > > -		typer = gicv3_read_typer(ptr + GICR_TYPER);
> > > > -		if ((typer >> 32) == aff) {
> > > > -			gicv3_redist_base() = ptr;
> > > > -			return;
> > > > -		}
> > > > -		ptr += stride; /* skip RD_base, SGI_base, etc. */
> > > > -	} while (!(typer & GICR_TYPER_LAST));
> > > > +	int i = 0;
> > > > +
> > > > +	while (gicv3_data.redist_bases[i]) {
> > > 
> > > I don't get this.  We never change I?
> > > 
> > > (conincidentally, my test of gicv3-ipi never completes, I wonder if
> > > these concepts are related...)
> > 
> > Darn it. I dropped the '++i' I had in v1. My regression testing naturally
> > only needs i=0 (I don't have anything with > 123 cpus to test on), so the
> > problem didn't show up for me.
> > 
> > Maybe you can add a ++i in and see if it works for you before I send v3?
> > If it's still busted I'll have to stop shotgunning code, and maybe even
> > find a way to test it myself.
> > 
> 
> Indeed, works perfectly.  With an added i++:
> 
> Tested-by: Christoffer Dall <christoffer.dall@arm.com>

Thanks for the additionally testing Christoffer. I'll send v3 after
lunch.

drew