Hi Julien, > On 3 May 2022, at 4:52 pm, Julien Grall <[email protected]> wrote: > > > > On 28/04/2022 15:11, Rahul Singh wrote: >> Hi Julien, > > Hi Rahul, > >>> 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. > > 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
