| Message ID | 7762e8e35be1f99f2a7ca81aa8cf8fc502030e7b.1651075773.git.rahul.singh@arm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | arm/its: enable LPIs before mapping the collection table | expand |
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? > 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@arm.com> > --- > 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. However, given this works on some HW, can you clarify whether this is mandated by the spec or this is a bug in your HW? Cheers,
Hi Julien, > On 27 Apr 2022, at 6:59 pm, Julien Grall <julien@xen.org> 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} > >> 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@arm.com> >> --- >> 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. > > However, given this works on some HW, can you clarify whether this is mandated by the spec or this is a bug in your HW? Regards, Rahul
On 28/04/2022 11:00, Rahul Singh wrote: > Hi Julien, > >> On 27 Apr 2022, at 6:59 pm, Julien Grall <julien@xen.org> 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@arm.com> >>> --- >>> 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,
Hi Julien, > On 28 Apr 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: > > > > On 28/04/2022 11:00, Rahul Singh wrote: >> Hi Julien, >>> On 27 Apr 2022, at 6:59 pm, Julien Grall <julien@xen.org> 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@arm.com> >>>> --- >>>> 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
On 28/04/2022 15:11, Rahul Singh wrote: > Hi Julien, Hi Rahul, >> On 28 Apr 2022, at 1:59 pm, Julien Grall <julien@xen.org> wrote: >> >> >> >> On 28/04/2022 11:00, Rahul Singh wrote: >>> Hi Julien, >>>> On 27 Apr 2022, at 6:59 pm, Julien Grall <julien@xen.org> 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@arm.com> >>>>> --- >>>>> 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/ > > 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). > > To fix this issue, enable the LPIs using GICR_CTLR.EnableLPIs before > mapping the collection table. Cheers,
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 > 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@arm.com> > --- > 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; gicv3_enable_lpis() is using writel_relaxed(). So in theory, the write may not be visible before gicv3_its_setup_collection() send the command. So I think we need to add an smp_wmb() to ensure the ordering with a comment explaning why this is necessary. Cheers,
Hi Julien, > On 3 May 2022, at 4:52 pm, Julien Grall <julien@xen.org> 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@xen.org> wrote: >>> >>> >>> >>> On 28/04/2022 11:00, Rahul Singh wrote: >>>> Hi Julien, >>>>> On 27 Apr 2022, at 6:59 pm, Julien Grall <julien@xen.org> 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@arm.com> >>>>>> --- >>>>>> 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
Hi Julien, > On 3 May 2022, at 4:58 pm, Julien Grall <julien@xen.org> 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 >> 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@arm.com> >> --- >> 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; > > gicv3_enable_lpis() is using writel_relaxed(). So in theory, the write may not be visible before gicv3_its_setup_collection() send the command. Agree. > > So I think we need to add an smp_wmb() to ensure the ordering with a comment explaning why this is necessary. Or maybe be we can change the writer_relaxed() to writel() that will also work. - writel_relaxed(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR); + writel(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR); Regards, Rahul
Hi, On 04/05/2022 12:25, Rahul Singh wrote: >> On 3 May 2022, at 4:58 pm, Julien Grall <julien@xen.org> wrote: >> On 27/04/2022 17:14, Rahul Singh wrote: >>> MAPC_LPI_OFF ITS command error can be reported to software if LPIs are >>> 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@arm.com> >>> --- >>> 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; >> >> gicv3_enable_lpis() is using writel_relaxed(). So in theory, the write may not be visible before gicv3_its_setup_collection() send the command. > Agree. >> >> So I think we need to add an smp_wmb() to ensure the ordering with a comment explaning why this is necessary. > > Or maybe be we can change the writer_relaxed() to writel() that will also work. > > - writel_relaxed(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR); > + writel(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR); writel() guarantees the previous writes are observed before this write. But it would not guarantee that the write will be observed before the ones after. Also, after further thoughts, I think this wants to be wmb() (system-wide) rather than smp_wmb() (innershearable). Cheers,
Hi Julien, > On 4 May 2022, at 12:59 pm, Julien Grall <julien@xen.org> wrote: > > Hi, > > On 04/05/2022 12:25, Rahul Singh wrote: >>> On 3 May 2022, at 4:58 pm, Julien Grall <julien@xen.org> wrote: >>> On 27/04/2022 17:14, Rahul Singh wrote: >>>> MAPC_LPI_OFF ITS command error can be reported to software if LPIs are >>>> 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@arm.com> >>>> --- >>>> 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; >>> >>> gicv3_enable_lpis() is using writel_relaxed(). So in theory, the write may not be visible before gicv3_its_setup_collection() send the command. >> Agree. >>> >>> So I think we need to add an smp_wmb() to ensure the ordering with a comment explaning why this is necessary. >> Or maybe be we can change the writer_relaxed() to writel() that will also work. >> - writel_relaxed(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR); >> + writel(val | GICR_CTLR_ENABLE_LPIS, GICD_RDIST_BASE + GICR_CTLR); > > writel() guarantees the previous writes are observed before this write. But it would not guarantee that the write will be observed before the ones after. > > Also, after further thoughts, I think this wants to be wmb() (system-wide) rather than smp_wmb() (innershearable). > Ok. I will use the wmb() and will send the next version. Regards, Rahul
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; } /* Set priority on PPI and SGI interrupts */
MAPC_LPI_OFF ITS command error can be reported to software if LPIs are 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@arm.com> --- xen/arch/arm/gic-v3.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)