Re: [kvm-unit-tests PATCH v6 11/11] arm/arm64: gic: don't just use zero

[Andrew Jones]

On Wed, Nov 23, 2016 at 02:33:31PM +0100, Auger Eric wrote:
> 
> 
> On 23/11/2016 14:01, Andrew Jones wrote:
> > On Wed, Nov 23, 2016 at 12:28:34PM +0100, Auger Eric wrote:
> >> Hi,
> >>
> >> On 14/11/2016 22:08, Andrew Jones wrote:
> >>> Allow user to select who sends ipis and with which irq,
> >>> rather than just always sending irq=0 from cpu0.
> >> From a user point of view is there a way to know the list of available
> >> tests and their arg?
> > 
> > Not at the moment. I could change the arg-less run and/or add
> > a '-h' though that would provide a usage. So far unit tests
> > have never had -h's, but it's not a bad idea.
> 
> OK thanks for the info
> > 
> >>>
> >>> Signed-off-by: Andrew Jones 
> >>>
> >>> ---
> >>> v6:
> >>>  - make sender/irq names more future-proof [drew]
> >>>  - sanity check inputs [drew]
> >>>  - introduce check_sender/irq and bad_sender/irq to more
> >>>cleanly do checks [drew]
> >>>  - default sender and irq to 1, instead of still zero [drew]
> >>> v4: improve structure and make sure spurious checking is
> >>> done even when the sender isn't cpu0
> >>> v2: actually check that the irq received was the irq sent,
> >>> and (for gicv2) that the sender is the expected one.
> >>> ---
> >>>  arm/gic.c | 124 
> >>> +-
> >>>  1 file changed, 99 insertions(+), 25 deletions(-)
> >>>
> >>> diff --git a/arm/gic.c b/arm/gic.c
> >>> index d954a3775c26..638b8b140c96 100644
> >>> --- a/arm/gic.c
> >>> +++ b/arm/gic.c
> >>> @@ -11,6 +11,7 @@
> >>>   * This work is licensed under the terms of the GNU LGPL, version 2.
> >>>   */
> >>>  #include 
> >>> +#include 
> >>>  #include 
> >>>  #include 
> >>>  #include 
> >>> @@ -28,6 +29,8 @@ struct gic {
> >>>  
> >>>  static struct gic *gic;
> >>>  static int acked[NR_CPUS], spurious[NR_CPUS];
> >>> +static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
> >>> +static int cmdl_sender = 1, cmdl_irq = 1;
> >>>  static cpumask_t ready;
> >>>  
> >>>  static void nr_cpu_check(int nr)
> >>> @@ -43,10 +46,23 @@ static void wait_on_ready(void)
> >>>   cpu_relax();
> >>>  }
> >>>  
> >>> +static void stats_reset(void)
> >>> +{
> >>> + int i;
> >>> +
> >>> + for (i = 0; i < nr_cpus; ++i) {
> >>> + acked[i] = 0;
> >>> + bad_sender[i] = -1;
> >>> + bad_irq[i] = -1;
> >>> + }
> >>> + smp_wmb();
> >>> +}
> >>> +
> >>>  static void check_acked(cpumask_t *mask)
> >>>  {
> >>>   int missing = 0, extra = 0, unexpected = 0;
> >>>   int nr_pass, cpu, i;
> >>> + bool bad = false;
> >>>  
> >>>   /* Wait up to 5s for all interrupts to be delivered */
> >>>   for (i = 0; i < 50; ++i) {
> >>> @@ -56,9 +72,21 @@ static void check_acked(cpumask_t *mask)
> >>>   smp_rmb();
> >>>   nr_pass += cpumask_test_cpu(cpu, mask) ?
> >>>   acked[cpu] == 1 : acked[cpu] == 0;
> >>> +
> >>> + if (bad_sender[cpu] != -1) {
> >>> + printf("cpu%d received IPI from wrong sender 
> >>> %d\n",
> >>> + cpu, bad_sender[cpu]);
> >>> + bad = true;
> >>> + }
> >>> +
> >>> + if (bad_irq[cpu] != -1) {
> >>> + printf("cpu%d received wrong irq %d\n",
> >>> + cpu, bad_irq[cpu]);
> >>> + bad = true;
> >>> + }
> >>>   }
> >>>   if (nr_pass == nr_cpus) {
> >>> - report("Completed in %d ms", true, ++i * 100);
> >>> + report("Completed in %d ms", !bad, ++i * 100);
> >>>   return;
> >>>   }
> >>>   }
> >>> @@ -91,6 +119,22 @@ static void check_spurious(void)
> >>>   }
> >>>  }
> >>>  
> >>> +static void check_ipi_sender(u32 irqstat)
> >>> +{
> >>> + if (gic_version() == 2) {
> >>> + int src = (irqstat >> 10) & 7;
> >>> +
> >>> + if (src != cmdl_sender)
> >>> + bad_sender[smp_processor_id()] = src;
> >>> + }
> >>> +}
> >>> +
> >>> +static void check_irqnr(u32 irqnr)
> >>> +{
> >>> + if (irqnr != (u32)cmdl_irq)
> >>> + bad_irq[smp_processor_id()] = irqnr;
> >>> +}
> >>> +
> >>>  static void ipi_handler(struct pt_regs *regs __unused)
> >>>  {
> >>>   u32 irqstat = gic_read_iar();
> >>> @@ -98,8 +142,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
> >>>  
> >>>   if (irqnr != GICC_INT_SPURIOUS) {
> >>>   gic_write_eoir(irqstat);
> >>> - smp_rmb(); /* pairs with wmb in ipi_test functions */
> >>> + smp_rmb(); /* pairs with wmb in stats_reset */
> >>>   ++acked[smp_processor_id()];
> >>> + check_ipi_sender(irqstat);
> >>> + check_irqnr(irqnr);
> >>>   smp_wmb(); /* pairs with rmb in check_acked */
> >>>   } else {
> >>>   ++spurious[smp_processor_id()];
> >>> @@ -109,19 +155,19 @@ static void ipi_handler(struct pt_regs *regs 
> >>> 

Re: [kvm-unit-tests PATCH v6 11/11] arm/arm64: gic: don't just use zero

[Auger Eric]

On 23/11/2016 14:01, Andrew Jones wrote:
> On Wed, Nov 23, 2016 at 12:28:34PM +0100, Auger Eric wrote:
>> Hi,
>>
>> On 14/11/2016 22:08, Andrew Jones wrote:
>>> Allow user to select who sends ipis and with which irq,
>>> rather than just always sending irq=0 from cpu0.
>> From a user point of view is there a way to know the list of available
>> tests and their arg?
> 
> Not at the moment. I could change the arg-less run and/or add
> a '-h' though that would provide a usage. So far unit tests
> have never had -h's, but it's not a bad idea.

OK thanks for the info
> 
>>>
>>> Signed-off-by: Andrew Jones 
>>>
>>> ---
>>> v6:
>>>  - make sender/irq names more future-proof [drew]
>>>  - sanity check inputs [drew]
>>>  - introduce check_sender/irq and bad_sender/irq to more
>>>cleanly do checks [drew]
>>>  - default sender and irq to 1, instead of still zero [drew]
>>> v4: improve structure and make sure spurious checking is
>>> done even when the sender isn't cpu0
>>> v2: actually check that the irq received was the irq sent,
>>> and (for gicv2) that the sender is the expected one.
>>> ---
>>>  arm/gic.c | 124 
>>> +-
>>>  1 file changed, 99 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/arm/gic.c b/arm/gic.c
>>> index d954a3775c26..638b8b140c96 100644
>>> --- a/arm/gic.c
>>> +++ b/arm/gic.c
>>> @@ -11,6 +11,7 @@
>>>   * This work is licensed under the terms of the GNU LGPL, version 2.
>>>   */
>>>  #include 
>>> +#include 
>>>  #include 
>>>  #include 
>>>  #include 
>>> @@ -28,6 +29,8 @@ struct gic {
>>>  
>>>  static struct gic *gic;
>>>  static int acked[NR_CPUS], spurious[NR_CPUS];
>>> +static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
>>> +static int cmdl_sender = 1, cmdl_irq = 1;
>>>  static cpumask_t ready;
>>>  
>>>  static void nr_cpu_check(int nr)
>>> @@ -43,10 +46,23 @@ static void wait_on_ready(void)
>>> cpu_relax();
>>>  }
>>>  
>>> +static void stats_reset(void)
>>> +{
>>> +   int i;
>>> +
>>> +   for (i = 0; i < nr_cpus; ++i) {
>>> +   acked[i] = 0;
>>> +   bad_sender[i] = -1;
>>> +   bad_irq[i] = -1;
>>> +   }
>>> +   smp_wmb();
>>> +}
>>> +
>>>  static void check_acked(cpumask_t *mask)
>>>  {
>>> int missing = 0, extra = 0, unexpected = 0;
>>> int nr_pass, cpu, i;
>>> +   bool bad = false;
>>>  
>>> /* Wait up to 5s for all interrupts to be delivered */
>>> for (i = 0; i < 50; ++i) {
>>> @@ -56,9 +72,21 @@ static void check_acked(cpumask_t *mask)
>>> smp_rmb();
>>> nr_pass += cpumask_test_cpu(cpu, mask) ?
>>> acked[cpu] == 1 : acked[cpu] == 0;
>>> +
>>> +   if (bad_sender[cpu] != -1) {
>>> +   printf("cpu%d received IPI from wrong sender 
>>> %d\n",
>>> +   cpu, bad_sender[cpu]);
>>> +   bad = true;
>>> +   }
>>> +
>>> +   if (bad_irq[cpu] != -1) {
>>> +   printf("cpu%d received wrong irq %d\n",
>>> +   cpu, bad_irq[cpu]);
>>> +   bad = true;
>>> +   }
>>> }
>>> if (nr_pass == nr_cpus) {
>>> -   report("Completed in %d ms", true, ++i * 100);
>>> +   report("Completed in %d ms", !bad, ++i * 100);
>>> return;
>>> }
>>> }
>>> @@ -91,6 +119,22 @@ static void check_spurious(void)
>>> }
>>>  }
>>>  
>>> +static void check_ipi_sender(u32 irqstat)
>>> +{
>>> +   if (gic_version() == 2) {
>>> +   int src = (irqstat >> 10) & 7;
>>> +
>>> +   if (src != cmdl_sender)
>>> +   bad_sender[smp_processor_id()] = src;
>>> +   }
>>> +}
>>> +
>>> +static void check_irqnr(u32 irqnr)
>>> +{
>>> +   if (irqnr != (u32)cmdl_irq)
>>> +   bad_irq[smp_processor_id()] = irqnr;
>>> +}
>>> +
>>>  static void ipi_handler(struct pt_regs *regs __unused)
>>>  {
>>> u32 irqstat = gic_read_iar();
>>> @@ -98,8 +142,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
>>>  
>>> if (irqnr != GICC_INT_SPURIOUS) {
>>> gic_write_eoir(irqstat);
>>> -   smp_rmb(); /* pairs with wmb in ipi_test functions */
>>> +   smp_rmb(); /* pairs with wmb in stats_reset */
>>> ++acked[smp_processor_id()];
>>> +   check_ipi_sender(irqstat);
>>> +   check_irqnr(irqnr);
>>> smp_wmb(); /* pairs with rmb in check_acked */
>>> } else {
>>> ++spurious[smp_processor_id()];
>>> @@ -109,19 +155,19 @@ static void ipi_handler(struct pt_regs *regs __unused)
>>>  
>>>  static void gicv2_ipi_send_self(void)
>>>  {
>>> -   writel(2 << 24, gicv2_dist_base() + GICD_SGIR);
>>> +   writel(2 << 24 | cmdl_irq, gicv2_dist_base() + GICD_SGIR);
>>>  }
>>>  
>>> -static void gicv2_ipi_send_tlist(cpumask_t 

Re: [kvm-unit-tests PATCH v6 11/11] arm/arm64: gic: don't just use zero

[Andrew Jones]

On Wed, Nov 23, 2016 at 12:28:34PM +0100, Auger Eric wrote:
> Hi,
> 
> On 14/11/2016 22:08, Andrew Jones wrote:
> > Allow user to select who sends ipis and with which irq,
> > rather than just always sending irq=0 from cpu0.
> From a user point of view is there a way to know the list of available
> tests and their arg?

Not at the moment. I could change the arg-less run and/or add
a '-h' though that would provide a usage. So far unit tests
have never had -h's, but it's not a bad idea.

> > 
> > Signed-off-by: Andrew Jones 
> > 
> > ---
> > v6:
> >  - make sender/irq names more future-proof [drew]
> >  - sanity check inputs [drew]
> >  - introduce check_sender/irq and bad_sender/irq to more
> >cleanly do checks [drew]
> >  - default sender and irq to 1, instead of still zero [drew]
> > v4: improve structure and make sure spurious checking is
> > done even when the sender isn't cpu0
> > v2: actually check that the irq received was the irq sent,
> > and (for gicv2) that the sender is the expected one.
> > ---
> >  arm/gic.c | 124 
> > +-
> >  1 file changed, 99 insertions(+), 25 deletions(-)
> > 
> > diff --git a/arm/gic.c b/arm/gic.c
> > index d954a3775c26..638b8b140c96 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -11,6 +11,7 @@
> >   * This work is licensed under the terms of the GNU LGPL, version 2.
> >   */
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -28,6 +29,8 @@ struct gic {
> >  
> >  static struct gic *gic;
> >  static int acked[NR_CPUS], spurious[NR_CPUS];
> > +static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
> > +static int cmdl_sender = 1, cmdl_irq = 1;
> >  static cpumask_t ready;
> >  
> >  static void nr_cpu_check(int nr)
> > @@ -43,10 +46,23 @@ static void wait_on_ready(void)
> > cpu_relax();
> >  }
> >  
> > +static void stats_reset(void)
> > +{
> > +   int i;
> > +
> > +   for (i = 0; i < nr_cpus; ++i) {
> > +   acked[i] = 0;
> > +   bad_sender[i] = -1;
> > +   bad_irq[i] = -1;
> > +   }
> > +   smp_wmb();
> > +}
> > +
> >  static void check_acked(cpumask_t *mask)
> >  {
> > int missing = 0, extra = 0, unexpected = 0;
> > int nr_pass, cpu, i;
> > +   bool bad = false;
> >  
> > /* Wait up to 5s for all interrupts to be delivered */
> > for (i = 0; i < 50; ++i) {
> > @@ -56,9 +72,21 @@ static void check_acked(cpumask_t *mask)
> > smp_rmb();
> > nr_pass += cpumask_test_cpu(cpu, mask) ?
> > acked[cpu] == 1 : acked[cpu] == 0;
> > +
> > +   if (bad_sender[cpu] != -1) {
> > +   printf("cpu%d received IPI from wrong sender 
> > %d\n",
> > +   cpu, bad_sender[cpu]);
> > +   bad = true;
> > +   }
> > +
> > +   if (bad_irq[cpu] != -1) {
> > +   printf("cpu%d received wrong irq %d\n",
> > +   cpu, bad_irq[cpu]);
> > +   bad = true;
> > +   }
> > }
> > if (nr_pass == nr_cpus) {
> > -   report("Completed in %d ms", true, ++i * 100);
> > +   report("Completed in %d ms", !bad, ++i * 100);
> > return;
> > }
> > }
> > @@ -91,6 +119,22 @@ static void check_spurious(void)
> > }
> >  }
> >  
> > +static void check_ipi_sender(u32 irqstat)
> > +{
> > +   if (gic_version() == 2) {
> > +   int src = (irqstat >> 10) & 7;
> > +
> > +   if (src != cmdl_sender)
> > +   bad_sender[smp_processor_id()] = src;
> > +   }
> > +}
> > +
> > +static void check_irqnr(u32 irqnr)
> > +{
> > +   if (irqnr != (u32)cmdl_irq)
> > +   bad_irq[smp_processor_id()] = irqnr;
> > +}
> > +
> >  static void ipi_handler(struct pt_regs *regs __unused)
> >  {
> > u32 irqstat = gic_read_iar();
> > @@ -98,8 +142,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
> >  
> > if (irqnr != GICC_INT_SPURIOUS) {
> > gic_write_eoir(irqstat);
> > -   smp_rmb(); /* pairs with wmb in ipi_test functions */
> > +   smp_rmb(); /* pairs with wmb in stats_reset */
> > ++acked[smp_processor_id()];
> > +   check_ipi_sender(irqstat);
> > +   check_irqnr(irqnr);
> > smp_wmb(); /* pairs with rmb in check_acked */
> > } else {
> > ++spurious[smp_processor_id()];
> > @@ -109,19 +155,19 @@ static void ipi_handler(struct pt_regs *regs __unused)
> >  
> >  static void gicv2_ipi_send_self(void)
> >  {
> > -   writel(2 << 24, gicv2_dist_base() + GICD_SGIR);
> > +   writel(2 << 24 | cmdl_irq, gicv2_dist_base() + GICD_SGIR);
> >  }
> >  
> > -static void gicv2_ipi_send_tlist(cpumask_t *mask, int irq __unused)
> > +static void gicv2_ipi_send_tlist(cpumask_t *mask, int 

Re: [kvm-unit-tests PATCH v6 11/11] arm/arm64: gic: don't just use zero

[Auger Eric]

Hi,

On 14/11/2016 22:08, Andrew Jones wrote:
> Allow user to select who sends ipis and with which irq,
> rather than just always sending irq=0 from cpu0.
>From a user point of view is there a way to know the list of available
tests and their arg?
> 
> Signed-off-by: Andrew Jones 
> 
> ---
> v6:
>  - make sender/irq names more future-proof [drew]
>  - sanity check inputs [drew]
>  - introduce check_sender/irq and bad_sender/irq to more
>cleanly do checks [drew]
>  - default sender and irq to 1, instead of still zero [drew]
> v4: improve structure and make sure spurious checking is
> done even when the sender isn't cpu0
> v2: actually check that the irq received was the irq sent,
> and (for gicv2) that the sender is the expected one.
> ---
>  arm/gic.c | 124 
> +-
>  1 file changed, 99 insertions(+), 25 deletions(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index d954a3775c26..638b8b140c96 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -11,6 +11,7 @@
>   * This work is licensed under the terms of the GNU LGPL, version 2.
>   */
>  #include 
> +#include 
>  #include 
>  #include 
>  #include 
> @@ -28,6 +29,8 @@ struct gic {
>  
>  static struct gic *gic;
>  static int acked[NR_CPUS], spurious[NR_CPUS];
> +static int bad_sender[NR_CPUS], bad_irq[NR_CPUS];
> +static int cmdl_sender = 1, cmdl_irq = 1;
>  static cpumask_t ready;
>  
>  static void nr_cpu_check(int nr)
> @@ -43,10 +46,23 @@ static void wait_on_ready(void)
>   cpu_relax();
>  }
>  
> +static void stats_reset(void)
> +{
> + int i;
> +
> + for (i = 0; i < nr_cpus; ++i) {
> + acked[i] = 0;
> + bad_sender[i] = -1;
> + bad_irq[i] = -1;
> + }
> + smp_wmb();
> +}
> +
>  static void check_acked(cpumask_t *mask)
>  {
>   int missing = 0, extra = 0, unexpected = 0;
>   int nr_pass, cpu, i;
> + bool bad = false;
>  
>   /* Wait up to 5s for all interrupts to be delivered */
>   for (i = 0; i < 50; ++i) {
> @@ -56,9 +72,21 @@ static void check_acked(cpumask_t *mask)
>   smp_rmb();
>   nr_pass += cpumask_test_cpu(cpu, mask) ?
>   acked[cpu] == 1 : acked[cpu] == 0;
> +
> + if (bad_sender[cpu] != -1) {
> + printf("cpu%d received IPI from wrong sender 
> %d\n",
> + cpu, bad_sender[cpu]);
> + bad = true;
> + }
> +
> + if (bad_irq[cpu] != -1) {
> + printf("cpu%d received wrong irq %d\n",
> + cpu, bad_irq[cpu]);
> + bad = true;
> + }
>   }
>   if (nr_pass == nr_cpus) {
> - report("Completed in %d ms", true, ++i * 100);
> + report("Completed in %d ms", !bad, ++i * 100);
>   return;
>   }
>   }
> @@ -91,6 +119,22 @@ static void check_spurious(void)
>   }
>  }
>  
> +static void check_ipi_sender(u32 irqstat)
> +{
> + if (gic_version() == 2) {
> + int src = (irqstat >> 10) & 7;
> +
> + if (src != cmdl_sender)
> + bad_sender[smp_processor_id()] = src;
> + }
> +}
> +
> +static void check_irqnr(u32 irqnr)
> +{
> + if (irqnr != (u32)cmdl_irq)
> + bad_irq[smp_processor_id()] = irqnr;
> +}
> +
>  static void ipi_handler(struct pt_regs *regs __unused)
>  {
>   u32 irqstat = gic_read_iar();
> @@ -98,8 +142,10 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  
>   if (irqnr != GICC_INT_SPURIOUS) {
>   gic_write_eoir(irqstat);
> - smp_rmb(); /* pairs with wmb in ipi_test functions */
> + smp_rmb(); /* pairs with wmb in stats_reset */
>   ++acked[smp_processor_id()];
> + check_ipi_sender(irqstat);
> + check_irqnr(irqnr);
>   smp_wmb(); /* pairs with rmb in check_acked */
>   } else {
>   ++spurious[smp_processor_id()];
> @@ -109,19 +155,19 @@ static void ipi_handler(struct pt_regs *regs __unused)
>  
>  static void gicv2_ipi_send_self(void)
>  {
> - writel(2 << 24, gicv2_dist_base() + GICD_SGIR);
> + writel(2 << 24 | cmdl_irq, gicv2_dist_base() + GICD_SGIR);
>  }
>  
> -static void gicv2_ipi_send_tlist(cpumask_t *mask, int irq __unused)
> +static void gicv2_ipi_send_tlist(cpumask_t *mask, int irq)
>  {
>   u8 tlist = (u8)cpumask_bits(mask)[0];
>  
> - writel(tlist << 16, gicv2_dist_base() + GICD_SGIR);
> + writel(tlist << 16 | irq, gicv2_dist_base() + GICD_SGIR);
>  }
>  
>  static void gicv2_ipi_send_broadcast(void)
>  {
> - writel(1 << 24, gicv2_dist_base() + GICD_SGIR);
> + writel(1 << 24 | cmdl_irq, gicv2_dist_base() + GICD_SGIR);
>  }
>  
>  static void