Re: [PATCH] irqchip/gic: Don't write to GICD_ICFGR0

[Mikko Perttunen]

On 07.04.2017 10:32, Marc Zyngier wrote:
> On 07/04/17 07:49, Mikko Perttunen wrote:
> > On 06.04.2017 12:26, Marc Zyngier wrote:
> > > On 06/04/17 09:17, Mikko Perttunen wrote:
> > > > From: Matt Craighead <mcraighead@xxxxxxxxxx>
> > > >
> > > > According to the GICv2 specification, the GICD_ICFGR0,
> > > > or GIC_DIST_CONFIG[0] register is read-only. Therefore
> > > > avoid writing to it.
> > >
> > > Have you verified that this also applies to pre-v2 GICs?
> >
> > I had not, but I just looked up the GICv1 specification and this also
> > applies to GICv1.
> >
> > > > Signed-off-by: Matt Craighead <mcraighead@xxxxxxxxxx>
> > > > [mperttunen@xxxxxxxxxx: commit message rewritten]
> > > > Signed-off-by: Mikko Perttunen <mperttunen@xxxxxxxxxx>
> > > > ---
> > > >  drivers/irqchip/irq-gic.c | 4 ++--
> > > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/drivers/irqchip/irq-gic.c b/drivers/irqchip/irq-gic.c
> > > > index 1b1df4f770bd..d9c0000050e0 100644
> > > > --- a/drivers/irqchip/irq-gic.c
> > > > +++ b/drivers/irqchip/irq-gic.c
> > > > @@ -609,7 +609,7 @@ void gic_dist_restore(struct gic_chip_data *gic)
> > > >
> > > >  	writel_relaxed(GICD_DISABLE, dist_base + GIC_DIST_CTRL);
> > > >
> > > > -	for (i = 0; i < DIV_ROUND_UP(gic_irqs, 16); i++)
> > > > +	for (i = 1; i < DIV_ROUND_UP(gic_irqs, 16); i++)
> > > >  		writel_relaxed(gic->saved_spi_conf[i],
> > > >  			dist_base + GIC_DIST_CONFIG + i * 4);
> > > >
> > > > @@ -699,7 +699,7 @@ void gic_cpu_restore(struct gic_chip_data *gic)
> > > >  	}
> > > >
> > > >  	ptr = raw_cpu_ptr(gic->saved_ppi_conf);
> > > > -	for (i = 0; i < DIV_ROUND_UP(32, 16); i++)
> > > > +	for (i = 1; i < DIV_ROUND_UP(32, 16); i++)
> > > >  		writel_relaxed(ptr[i], dist_base + GIC_DIST_CONFIG + i * 4);
> > >
> > > Assuming that the above stands for all GICs, it feels like there is room
> > > for simplification here. But you haven't dealt with the save side, so
> > > what's the point?
> >
> > Yes, with this we could also drop saving the value when saving, and
> > that's probably worth doing. We could also just shift the indexing to be
> > one higher always.
> >
> > > Also, you're missing out some other stuff which is (by definition) RO as
> > > well, such as the target registers for SGIs and PPIs. Finally, there is
> > > the question of the allocated memory for these registers.
> >
> > At least for the target register, the driver already seems to have code
> > to skip the fields defined as read-only. I havent looked for other
> > read-only registers, but this is the only registers we are having issues
> > with (see below).
> >
> > > Overall, I'm not sure what this patch is trying to achieve. It doesn't
> > > fix a bug, and is not complete enough to do something useful (even
> > > though it would only be saving a handful of bytes).
> > >
> > > Maybe you can explain what you're trying to do here?
> >
> > Sure. Our simulation environment enforces the read-only-ness of these
> > registers, so the driver as is doesn't work in simulation. As far as I
> > understand, the register being read-only means that the model is allowed
> > to do this.
>
> I'm not sure this is a valid model for a GICv2. Some other parts of the
> documentation hint at registers being RO/WI, but more crucially, there
> is the case of GICD_ICFGR1. It is implementation defined whether it is
> RO or not, and SW has no way to find out other than writing to it. What
> would you do in this case? My position is that GICD_GICR0 should have a
> similar behaviour.
>
> Thoughts?

We could add some device data that specifies this, but that that would 
be serious overkill just to support a simulation implementation. I do 
think we could still make the change for ICFGR0, as that is guaranteed 
to be read-only and the patch is very small and correct regardless of 
the interpretation of 'read-only'. However, I do see your point of 
keeping it uniform. Of course, it's your decision :)

> 	M.

Thanks,
Mikko