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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 28 Apr 2022, at 1:59 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> 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.
Ok.
> 
>>> 
>>>> 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).

Ok. I will modify the commit msg as below.Please let me know if it is okay.

arm/its: enable LPIs before mapping the collection table

When Xen boots on the platform that implements the GIC 600, ITS
MAPC_LPI_OFF uncorrectable command error issue is oberved.

As per the GIC-600 TRM (Revision: r1p6) MAPC_LPI_OFF command error can
be reported if the ITS MAPC command has tried to map a collection to a core
that does not have LPIs enabled.

To fix this issue, enable the LPIs using GICR_CTLR.EnableLPIs before
mapping the collection table.

Regards,
Rahul