Re: [PATCH] irqchip/gic-v4: Fix non-stick page size error for setup GITS_BASER

[Marc Zyngier]

Hi Shaokun,

On 2020-03-16 05:48, Shaokun Zhang wrote:
> Hi Marcï
>
> On 2020/3/13 20:00, Marc Zyngier wrote:
> > Shaokun, Nianyao,
> >
> > On 2020-03-13 08:46, Shaokun Zhang wrote:
> > > From: Nianyao Tang <tangnianyao@xxxxxxxxxx>
> > >
> > > There is an error when ITS is probed if hw GITS_BASER<2>
> > > only supports psz=SZ_16K while GITS_BASER<1> only supports psz=SZ_4K.
> > > In its_alloc_tables, it has updated GITS_BASER<1>.psz and uses
> > > psz=SZ_4K for setup GITS_BASER<2>, and would fail in writing
> > > GITS_BASER<2>.psz=SZ_4K. 4K PAGE is the smallest and shall stop retry
> > > for other page sizes.
> > >
> > > That latter GITS_BASER<n>.psz is larger than former, will lead
> > > to similar error.
> > >
> > > [    0.000000] ITS@0x0000000660000000: Virtual CPUs doesn't stick:
> > > ba1f0824404004ff ba1f0824404005ff
> > > [    0.000000] ITS@0x0000000660000000: failed probing (-6)
> > > [    0.000000] ITS: No ITS available, not enabling LPIs
> > >
> > > From GIC SPEC document, it's allowed for this implematation, the 
> > > latter
> > > GITS_BASER<n>.psz is larger than the former.
> >
> > I was really hoping nobody would build an ITS with disjoint sets of
> > page sizes. Oh well...
> >
> > > Let's fix this error with following patch.
> > >
> > > Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> > > Cc: Marc Zyngier <maz@xxxxxxxxxx>
> > > Signed-off-by: Nianyao Tang <tangnianyao@xxxxxxxxxx>
> > > Signed-off-by: Shaokun Zhang <zhangshaokun@xxxxxxxxxxxxx>
> > > ---
> > >  drivers/irqchip/irq-gic-v3-its.c | 1 -
> > >  1 file changed, 1 deletion(-)
> > >
> > > diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> > > b/drivers/irqchip/irq-gic-v3-its.c
> > > index 83b1186..59bf8d6 100644
> > > --- a/drivers/irqchip/irq-gic-v3-its.c
> > > +++ b/drivers/irqchip/irq-gic-v3-its.c
> > > @@ -2341,7 +2341,6 @@ static int its_alloc_tables(struct its_node 
> > > *its)
> > >          }
> > >
> > >          /* Update settings which will be used for next BASERn */
> > > -        psz = baser->psz;
> > >          cache = baser->val & GITS_BASER_CACHEABILITY_MASK;
> > >          shr = baser->val & GITS_BASER_SHAREABILITY_MASK;
> > >      }
> >
> > I think this just papers over the problem, and I'd rather tackle it 
> > fully
> > by forcing the probe of the supported page sizes for each GITS_BASERn
> > register. See the patch below (which I've only boot-tested once on 
> > pretty
> > standard HW as well as a guest).
> >
> > This, in turn, simplifies the way we perform the allocation (no nested
> > retry loop). This is of course a much more patch invasive, but at 
> > least
> > it doesn't leave too much cruft behind.
> >
> > Please let me know whether this solves your issue.
>
> Thanks your quick reply.
> With your following fixed patch, it can work for Nianyao, So:
> Tested-by: Nianyao Tang <tangnianyao@xxxxxxxxxx>

Thanks for the quick feedback. I've now added this patch to the 5.7 
queue.

        M.
--
Jazz is not dead. It just smells funny...