Re: [PATCH] arm/its: enable LPIs before mapping the collection table

[Julien Grall <julien@xxxxxxx>]

Hi,

On 04/05/2022 12:25, Rahul Singh wrote:
> > On 3 May 2022, at 4:58 pm, Julien Grall <julien@xxxxxxx> wrote:
> > On 27/04/2022 17:14, Rahul Singh wrote:
> > > MAPC_LPI_OFF ITS command error can be reported to software if LPIs are
> > > not enabled before mapping the collection table using MAPC command.
> > > Enable the LPIs using GICR_CTLR.EnableLPIs before mapping the collection
> > > table.
> > > Signed-off-by: Rahul Singh <rahul.singh@xxxxxxx>
> > > ---
> > > xen/arch/arm/gic-v3.c | 4 ++--
> > > 1 file changed, 2 insertions(+), 2 deletions(-)
> > > diff --git a/xen/arch/arm/gic-v3.c b/xen/arch/arm/gic-v3.c
> > > index 3c472ed768..8fb0014b16 100644
> > > --- a/xen/arch/arm/gic-v3.c
> > > +++ b/xen/arch/arm/gic-v3.c
> > > @@ -812,11 +812,11 @@ static int gicv3_cpu_init(void)
> > > /* If the host has any ITSes, enable LPIs now. */
> > > if ( gicv3_its_host_has_its() )
> > > {
> > > + if ( !gicv3_enable_lpis() )
> > > + return -EBUSY;
> > gicv3_enable_lpis() is using writel_relaxed(). So in theory, the write may not 
> > be visible before gicv3_its_setup_collection() send the command.
> Agree.
> > So I think we need to add an smp_wmb() to ensure the ordering with a comment 
> > explaning why this is necessary.
> Or maybe be we can change the writer_relaxed() to writel() that will also work.
>
> -    writel_relaxed(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR);
> +    writel(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR);
writel() guarantees the previous writes are observed before this write. 
But it would not guarantee that the write will be observed before the 
ones after.
Also, after further thoughts, I think this wants to be wmb() 
(system-wide) rather than smp_wmb() (innershearable).
Cheers,

--
Julien Grall