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

[Rahul Singh <Rahul.Singh@xxxxxxx>]

Hi Julien,

> On 3 May 2022, at 4:52 pm, Julien Grall <julien@xxxxxxx> wrote:
> 
> 
> 
> On 28/04/2022 15:11, Rahul Singh wrote:
>> Hi Julien,
> 
> Hi Rahul,
> 
>>> 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.
> 
> s/oberved/observed/
Ack. 
> 
>> 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.
> 
> Please add a quote from the GICv3 specification (see my previous reply).
Ok.

Regards,
Rahul