[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH] arm/its: enable LPIs before mapping the collection table
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/
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,
--
Julien Grall
|
