[PATCH kvm-unit-tests] arm/arm64: fix gicv3-active test

[PATCH kvm-unit-tests] arm/arm64: fix gicv3-active test

[Andrew Jones]

GICR_ICACTIVER0 (aka GICD_ICACTIVER) is based of the SGI_base,
not the RD_base.

Signed-off-by: Andrew Jones <drjones@redhat.com>
---
 arm/gic.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arm/gic.c b/arm/gic.c
index a945f7ab8385..5dd958e8b66b 100644
--- a/arm/gic.c
+++ b/arm/gic.c
@@ -274,7 +274,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
 		if (gic_version() == 2)
 			base = gicv2_dist_base();
 		else
-			base = gicv3_redist_base();
+			base = gicv3_sgi_base();
 
 		writel(val, base + GICD_ICACTIVER);
 
-- 
2.13.6

Re: [PATCH kvm-unit-tests] arm/arm64: fix gicv3-active test

[Christoffer Dall]

On Tue, Jan 09, 2018 at 07:54:29PM +0100, Andrew Jones wrote:
> GICR_ICACTIVER0 (aka GICD_ICACTIVER) is based of the SGI_base,
> not the RD_base.
> 
> Signed-off-by: Andrew Jones <drjones@redhat.com>
> ---
>  arm/gic.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/arm/gic.c b/arm/gic.c
> index a945f7ab8385..5dd958e8b66b 100644
> --- a/arm/gic.c
> +++ b/arm/gic.c
> @@ -274,7 +274,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
>  		if (gic_version() == 2)
>  			base = gicv2_dist_base();
>  		else
> -			base = gicv3_redist_base();
> +			base = gicv3_sgi_base();

Shouldn't this be:
		if (gic_version() == 2)
			base = gicv2_dist_base();
		else if (irqnr < GIC_NR_PRIVATE_IRQS)
			base = gicv3_sgi_base();
		else
			base = gicv3_dist_base();


Which would support all IRQs ?

Thanks,
-Christoffer

>  
>  		writel(val, base + GICD_ICACTIVER);
>  
> -- 
> 2.13.6
> 

Re: [PATCH kvm-unit-tests] arm/arm64: fix gicv3-active test

[Andrew Jones]

On Tue, Jan 09, 2018 at 08:10:33PM +0100, Christoffer Dall wrote:
> On Tue, Jan 09, 2018 at 07:54:29PM +0100, Andrew Jones wrote:
> > GICR_ICACTIVER0 (aka GICD_ICACTIVER) is based of the SGI_base,
> > not the RD_base.
> > 
> > Signed-off-by: Andrew Jones <drjones@redhat.com>
> > ---
> >  arm/gic.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/arm/gic.c b/arm/gic.c
> > index a945f7ab8385..5dd958e8b66b 100644
> > --- a/arm/gic.c
> > +++ b/arm/gic.c
> > @@ -274,7 +274,7 @@ static void ipi_clear_active_handler(struct pt_regs *regs __unused)
> >  		if (gic_version() == 2)
> >  			base = gicv2_dist_base();
> >  		else
> > -			base = gicv3_redist_base();
> > +			base = gicv3_sgi_base();
> 
> Shouldn't this be:
> 		if (gic_version() == 2)
> 			base = gicv2_dist_base();
> 		else if (irqnr < GIC_NR_PRIVATE_IRQS)
> 			base = gicv3_sgi_base();
> 		else
> 			base = gicv3_dist_base();
> 
> 
> Which would support all IRQs ?

Yes, that would be the most general approach. Although, currently, the
unit test always uses an IPI with the SGI hard coded to 1 when generating
the interrupt. Of course I'm not opposed to extending the condition as
you've done above to prepare the handler for SPIs, but we'd need to
modify check_irqnr() as well, so I think we might as well just wait for
the addition of SPI tests to add/change everything at once.

Thanks,
drew