Hi Julien, > On 28 Apr 2022, at 1:59 pm, Julien Grall <[email protected]> wrote: > > > > On 28/04/2022 11:00, Rahul Singh wrote: >> Hi Julien, >>> On 27 Apr 2022, at 6:59 pm, Julien Grall <[email protected]> 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 <[email protected]> >>>> --- >>>> 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
