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

[Julien Grall <julien@xxxxxxx>]

On 28/04/2022 11:00, Rahul Singh wrote:
> Hi Julien,
>
> > On 27 Apr 2022, at 6:59 pm, Julien Grall <julien@xxxxxxx> wrote:
> >
> > Hi Rahul,
> >
> > On 27/04/2022 17:14, Rahul Singh wrote:
> > > MAPC_LPI_OFF ITS command error can be reported to software if LPIs are
> > Looking at the spec (ARM IHI 0069H), I can't find a command error named 
> > MAPC_LPI_OFF. Is it something specific to your HW?
> I found the issue on HW that implements GIC-600 and GIC-600 TRM specify the 
> MAPC_LPI_OFF its command error.
>
> https://developer.arm.com/documentation/100336/0106/introduction/about-the-gic-600
> {Table 3-15 ITS command and translation errors, records 13+ page 3-89}
Please provide a pointer to the spec in the commit message. This would 
help the reviewer to know where MAPC_LPI_OFF come from.
> > > 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;
> > > ret = gicv3_its_setup_collection(smp_processor_id());
> > > if ( ret )
> > > return ret;
> > > - if ( !gicv3_enable_lpis() )
> > > - return -EBUSY;
> > AFAICT, Linux is using the same ordering as your are proposing. It seems to 
> > have been introduced from the start, so it is not clear why we chose this 
> > approach.
> Yes I also confirmed that before sending the patch for review. I think this is 
> okay if we enable the enable LPIs before mapping the collection table.
In general, I expect change touching the GICv3 code based on the 
specification rather than "we think this is okay". This reduce the risk 
to make modification that could break other platforms (we can't possibly 
test all of them).
Reading through the spec, the definition of GICR.EnableLPIs contains the 
following:
"
0b0 LPI support is disabled. Any doorbell interrupt generated as a 
result of a write to a virtual LPI register must be discarded, and any 
ITS translation requests or commands involving LPIs in this 
Redistributor are ignored.
0b1 LPI support is enabled.
"

So your change is correct. But the commit message needs to be updated 
with more details on which GIC HW the issue was seen and why your 
proposal is correct (i.e. quoting the spec).
Cheers,

--
Julien Grall