Re: [RFC PATCH 01/33] irqchip/gic-v3: Add redistributor iterator

[Marc Zyngier]

On 16/02/17 12:47, Auger Eric wrote:
> Hi Marc,
> 
> On 17/01/2017 11:20, Marc Zyngier wrote:
>> In order to discover the VLPI properties, we need to iterate over
>> the redistributor regions. As we already have code that does this,
>> let's factor it out and make it slightly more generic.
>>
>> Signed-off-by: Marc Zyngier 
>> ---
>>  drivers/irqchip/irq-gic-v3.c | 77 
>> 
>>  1 file changed, 56 insertions(+), 21 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
>> index c132f29..5cadec0 100644
>> --- a/drivers/irqchip/irq-gic-v3.c
>> +++ b/drivers/irqchip/irq-gic-v3.c
>> @@ -421,24 +421,15 @@ static void __init gic_dist_init(void)
>>  gic_write_irouter(affinity, base + GICD_IROUTER + i * 8);
>>  }
>>  
>> -static int gic_populate_rdist(void)
>> +static int gic_scan_rdist_properties(int (*fn)(struct redist_region *,
>> +   void __iomem *))
>>  {
>> -unsigned long mpidr = cpu_logical_map(smp_processor_id());
>> -u64 typer;
>> -u32 aff;
>> +int ret = 0;
> in case
> if (reg != GIC_PIDR2_ARCH_GICv3 &&
>   reg != GIC_PIDR2_ARCH_GICv4) is true you now return 0. Don' 
> you
> want to return -ENODEV as before?
> 
> 
>>  int i;
>>  
>> -/*
>> - * Convert affinity to a 32bit value that can be matched to
>> - * GICR_TYPER bits [63:32].
>> - */
>> -aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
>> -   MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
>> -   MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
>> -   MPIDR_AFFINITY_LEVEL(mpidr, 0));
>> -
>>  for (i = 0; i < gic_data.nr_redist_regions; i++) {
>>  void __iomem *ptr = gic_data.redist_regions[i].redist_base;
>> +u64 typer;
>>  u32 reg;
>>  
>>  reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
>> @@ -450,14 +441,14 @@ static int gic_populate_rdist(void)
>>  
>>  do {
>>  typer = gic_read_typer(ptr + GICR_TYPER);
>> -if ((typer >> 32) == aff) {
>> -u64 offset = ptr - 
>> gic_data.redist_regions[i].redist_base;
>> -gic_data_rdist_rd_base() = ptr;
>> -gic_data_rdist()->phys_base = 
>> gic_data.redist_regions[i].phys_base + offset;
>> -pr_info("CPU%d: found redistributor %lx region 
>> %d:%pa\n",
>> -smp_processor_id(), mpidr, i,
>> -_data_rdist()->phys_base);
>> +ret = fn(gic_data.redist_regions + i, ptr);
>> +switch (ret) {
>> +case 0:
>>  return 0;
>> +case -1:
>> +break;
>> +default:
>> +ret = 0;
>>  }
>>  
>>  if (gic_data.redist_regions[i].single_redist)
>> @@ -473,9 +464,53 @@ static int gic_populate_rdist(void)
>>  } while (!(typer & GICR_TYPER_LAST));
>>  }
> Assuming you don't find the TYPER that matches the cpu (I don't know if
> it is possible), last fn() call will return 1 and the function will
> return 0. Before it returned -ENODEV I think.

Yeah, the whole thing was quite messy. I've (hopefully) fixed it now.

Thanks,

M.
-- 
Jazz is not dead. It just smells funny...
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [RFC PATCH 01/33] irqchip/gic-v3: Add redistributor iterator

[Auger Eric]

Hi Marc,

On 17/01/2017 11:20, Marc Zyngier wrote:
> In order to discover the VLPI properties, we need to iterate over
> the redistributor regions. As we already have code that does this,
> let's factor it out and make it slightly more generic.
> 
> Signed-off-by: Marc Zyngier 
> ---
>  drivers/irqchip/irq-gic-v3.c | 77 
> 
>  1 file changed, 56 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
> index c132f29..5cadec0 100644
> --- a/drivers/irqchip/irq-gic-v3.c
> +++ b/drivers/irqchip/irq-gic-v3.c
> @@ -421,24 +421,15 @@ static void __init gic_dist_init(void)
>   gic_write_irouter(affinity, base + GICD_IROUTER + i * 8);
>  }
>  
> -static int gic_populate_rdist(void)
> +static int gic_scan_rdist_properties(int (*fn)(struct redist_region *,
> +void __iomem *))
>  {
> - unsigned long mpidr = cpu_logical_map(smp_processor_id());
> - u64 typer;
> - u32 aff;
> + int ret = 0;
in case
if (reg != GIC_PIDR2_ARCH_GICv3 &&
reg != GIC_PIDR2_ARCH_GICv4) is true you now return 0. Don' 
you
want to return -ENODEV as before?


>   int i;
>  
> - /*
> -  * Convert affinity to a 32bit value that can be matched to
> -  * GICR_TYPER bits [63:32].
> -  */
> - aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> -MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> -MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
> -MPIDR_AFFINITY_LEVEL(mpidr, 0));
> -
>   for (i = 0; i < gic_data.nr_redist_regions; i++) {
>   void __iomem *ptr = gic_data.redist_regions[i].redist_base;
> + u64 typer;
>   u32 reg;
>  
>   reg = readl_relaxed(ptr + GICR_PIDR2) & GIC_PIDR2_ARCH_MASK;
> @@ -450,14 +441,14 @@ static int gic_populate_rdist(void)
>  
>   do {
>   typer = gic_read_typer(ptr + GICR_TYPER);
> - if ((typer >> 32) == aff) {
> - u64 offset = ptr - 
> gic_data.redist_regions[i].redist_base;
> - gic_data_rdist_rd_base() = ptr;
> - gic_data_rdist()->phys_base = 
> gic_data.redist_regions[i].phys_base + offset;
> - pr_info("CPU%d: found redistributor %lx region 
> %d:%pa\n",
> - smp_processor_id(), mpidr, i,
> - _data_rdist()->phys_base);
> + ret = fn(gic_data.redist_regions + i, ptr);
> + switch (ret) {
> + case 0:
>   return 0;
> + case -1:
> + break;
> + default:
> + ret = 0;
>   }
>  
>   if (gic_data.redist_regions[i].single_redist)
> @@ -473,9 +464,53 @@ static int gic_populate_rdist(void)
>   } while (!(typer & GICR_TYPER_LAST));
>   }
Assuming you don't find the TYPER that matches the cpu (I don't know if
it is possible), last fn() call will return 1 and the function will
return 0. Before it returned -ENODEV I think.

Thanks

Eric
>  
> + if (ret == -1)
> + ret = -ENODEV;
> +
> + return 0;
> +}
> +
> +static int __gic_populate_rdist(struct redist_region *region, void __iomem 
> *ptr)
> +{
> + unsigned long mpidr = cpu_logical_map(smp_processor_id());
> + u64 typer;
> + u32 aff;
> +
> + /*
> +  * Convert affinity to a 32bit value that can be matched to
> +  * GICR_TYPER bits [63:32].
> +  */
> + aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
> +MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
> +MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
> +MPIDR_AFFINITY_LEVEL(mpidr, 0));
> +
> + typer = gic_read_typer(ptr + GICR_TYPER);
> + if ((typer >> 32) == aff) {
> + u64 offset = ptr - region->redist_base;
> + gic_data_rdist_rd_base() = ptr;
> + gic_data_rdist()->phys_base = region->phys_base + offset;
> +
> + pr_info("CPU%d: found redistributor %lx region %d:%pa\n",
> + smp_processor_id(), mpidr,
> + (int)(region - gic_data.redist_regions),
> + _data_rdist()->phys_base);
> + return 0;
> + }
> +
> + /* Try next one */
> + return 1;
> +}
> +
> +static int gic_populate_rdist(void)
> +{
> + if (gic_scan_rdist_properties(__gic_populate_rdist) == 0)
> + return 0;
> +
>   /* We couldn't even deal with ourselves... */
>   WARN(true, "CPU%d: mpidr %lx has no re-distributor!\n",
> -  smp_processor_id(), mpidr);
> +  smp_processor_id(),
> +  (unsigned long)cpu_logical_map(smp_processor_id()));
>   return -ENODEV;
>  }
>  
> 

Re: [RFC PATCH 01/33] irqchip/gic-v3: Add redistributor iterator

[Shanker Donthineni]

Hi Marc,


On 01/17/2017 04:20 AM, Marc Zyngier wrote:

In order to discover the VLPI properties, we need to iterate over
the redistributor regions. As we already have code that does this,
let's factor it out and make it slightly more generic.

Signed-off-by: Marc Zyngier 
---
  drivers/irqchip/irq-gic-v3.c | 77

  1 file changed, 56 insertions(+), 21 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3.c b/drivers/irqchip/irq-gic-v3.c
index c132f29..5cadec0 100644
--- a/drivers/irqchip/irq-gic-v3.c
+++ b/drivers/irqchip/irq-gic-v3.c
@@ -421,24 +421,15 @@ static void __init gic_dist_init(void)
gic_write_irouter(affinity, base + GICD_IROUTER + i * 8);
  }
  
-static int gic_populate_rdist(void)

+static int gic_scan_rdist_properties(int (*fn)(struct redist_region *,
+  void __iomem *))
I don't see this function is parsing GICR properties, may be it makes 
readable on changing name to gic_redist_iterator().



  {
-   unsigned long mpidr = cpu_logical_map(smp_processor_id());
-   u64 typer;
-   u32 aff;
+   int ret = 0;
For readability purpose set  ret = ENODEV, to cover error case where 
gic_data.nr_redist_regions == 0.

int i;
  
-	/*

-* Convert affinity to a 32bit value that can be matched to
-* GICR_TYPER bits [63:32].
-*/
-   aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
-  MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
-  MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
-  MPIDR_AFFINITY_LEVEL(mpidr, 0));
-
for (i = 0; i < gic_data.nr_redist_regions; i++) {
void __iomem *ptr =
gic_data.redist_regions[i].redist_base;
+   u64 typer;
u32 reg;
  
  		reg = readl_relaxed(ptr + GICR_PIDR2) &

GIC_PIDR2_ARCH_MASK;
@@ -450,14 +441,14 @@ static int gic_populate_rdist(void)
  
  		do {

typer = gic_read_typer(ptr + GICR_TYPER);
-   if ((typer >> 32) == aff) {
-   u64 offset = ptr -
gic_data.redist_regions[i].redist_base;
-   gic_data_rdist_rd_base() = ptr;
-   gic_data_rdist()->phys_base =
gic_data.redist_regions[i].phys_base + offset;
-   pr_info("CPU%d: found redistributor %lx
region %d:%pa\n",
-   smp_processor_id(), mpidr, i,
-   _data_rdist()->phys_base);
+   ret = fn(gic_data.redist_regions + i, ptr);
+   switch (ret) {
+   case 0:
return 0;
+   case -1:
+   break;
+   default:
+   ret = 0;
}
  
  			if (gic_data.redist_regions[i].single_redist)

@@ -473,9 +464,53 @@ static int gic_populate_rdist(void)
} while (!(typer & GICR_TYPER_LAST));
  
  
+	if (ret == -1)

+   ret = -ENODEV;
+
__gic_populate_rdist() returns 1 to try next entry in the list. We 
should not return value 0 here if no matching entry is found otherwise 
the gic_populate_rdist() assumes that it found the corresponding GICR.


+   return 0;
+}
+
+static int __gic_populate_rdist(struct redist_region *region, void
__iomem *ptr)
+{
+   unsigned long mpidr = cpu_logical_map(smp_processor_id());
+   u64 typer;
+   u32 aff;
+
+   /*
+* Convert affinity to a 32bit value that can be matched to
+* GICR_TYPER bits [63:32].
+*/
+   aff = (MPIDR_AFFINITY_LEVEL(mpidr, 3) << 24 |
+  MPIDR_AFFINITY_LEVEL(mpidr, 2) << 16 |
+  MPIDR_AFFINITY_LEVEL(mpidr, 1) << 8 |
+  MPIDR_AFFINITY_LEVEL(mpidr, 0));
+
+   typer = gic_read_typer(ptr + GICR_TYPER);
+   if ((typer >> 32) == aff) {
+   u64 offset = ptr - region->redist_base;
+   gic_data_rdist_rd_base() = ptr;
+   gic_data_rdist()->phys_base = region->phys_base + offset;
+
+   pr_info("CPU%d: found redistributor %lx region %d:%pa\n",
+   smp_processor_id(), mpidr,
+   (int)(region - gic_data.redist_regions),
+   _data_rdist()->phys_base);
+   return 0;
+   }
+
+   /* Try next one */
+   return 1;
+}
+
+static int gic_populate_rdist(void)
+{
+   if (gic_scan_rdist_properties(__gic_populate_rdist) == 0)

what about 'if (!gic_scan_rdist_properties(__gic_populate_rdist))'?

+   return 0;
+
/* We couldn't even deal with ourselves... */
WARN(true, "CPU%d: mpidr %lx has no re-distributor!\n",
-smp_processor_id(), mpidr);
+smp_processor_id(),
+(unsigned long)cpu_logical_map(smp_processor_id()));
return -ENODEV;
  }
  

Re: [RFC PATCH 01/33] irqchip/gic-v3: Add redistributor iterator

[Thomas Gleixner]

On Tue, 17 Jan 2017, Marc Zyngier wrote:

> In order to discover the VLPI properties, we need to iterate over
> the redistributor regions. As we already have code that does this,
> let's factor it out and make it slightly more generic.
> 
> Signed-off-by: Marc Zyngier 

Reviewed-by: Thomas Gleixner 
___
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm