From: Nianyao Tang <[email protected]>
There is an error when ITS is probed if hw GITS_BASER<2>
only supports psz=SZ_16K while GITS_BASER<1> only supports psz=SZ_4K.
In its_alloc_tables, it has updated GITS_BASER<1>.psz and uses
psz=SZ_4K for setup GITS_BASER<2>, and would fail in writing
GITS_BASER<2>.psz=SZ_4K. 4K PAGE is the smallest and shall stop retry
for other page sizes.
That latter GITS_BASER<n>.psz is larger than former, will lead
to similar error.
[ 0.000000] ITS@0x0000000660000000: Virtual CPUs doesn't stick: ba1f0824404004ff ba1f0824404005ff
[ 0.000000] ITS@0x0000000660000000: failed probing (-6)
[ 0.000000] ITS: No ITS available, not enabling LPIs
From GIC SPEC document, it's allowed for this implematation, the latter
GITS_BASER<n>.psz is larger than the former.
Let's fix this error with following patch.
Cc: Thomas Gleixner <[email protected]>
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Nianyao Tang <[email protected]>
Signed-off-by: Shaokun Zhang <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 83b1186..59bf8d6 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2341,7 +2341,6 @@ static int its_alloc_tables(struct its_node *its)
}
/* Update settings which will be used for next BASERn */
- psz = baser->psz;
cache = baser->val & GITS_BASER_CACHEABILITY_MASK;
shr = baser->val & GITS_BASER_SHAREABILITY_MASK;
}
--
1.9.1
Shaokun, Nianyao,
On 2020-03-13 08:46, Shaokun Zhang wrote:
> From: Nianyao Tang <[email protected]>
>
> There is an error when ITS is probed if hw GITS_BASER<2>
> only supports psz=SZ_16K while GITS_BASER<1> only supports psz=SZ_4K.
> In its_alloc_tables, it has updated GITS_BASER<1>.psz and uses
> psz=SZ_4K for setup GITS_BASER<2>, and would fail in writing
> GITS_BASER<2>.psz=SZ_4K. 4K PAGE is the smallest and shall stop retry
> for other page sizes.
>
> That latter GITS_BASER<n>.psz is larger than former, will lead
> to similar error.
>
> [ 0.000000] ITS@0x0000000660000000: Virtual CPUs doesn't stick:
> ba1f0824404004ff ba1f0824404005ff
> [ 0.000000] ITS@0x0000000660000000: failed probing (-6)
> [ 0.000000] ITS: No ITS available, not enabling LPIs
>
> From GIC SPEC document, it's allowed for this implematation, the latter
> GITS_BASER<n>.psz is larger than the former.
I was really hoping nobody would build an ITS with disjoint sets of
page sizes. Oh well...
> Let's fix this error with following patch.
>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Signed-off-by: Nianyao Tang <[email protected]>
> Signed-off-by: Shaokun Zhang <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index 83b1186..59bf8d6 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2341,7 +2341,6 @@ static int its_alloc_tables(struct its_node *its)
> }
>
> /* Update settings which will be used for next BASERn */
> - psz = baser->psz;
> cache = baser->val & GITS_BASER_CACHEABILITY_MASK;
> shr = baser->val & GITS_BASER_SHAREABILITY_MASK;
> }
I think this just papers over the problem, and I'd rather tackle it
fully
by forcing the probe of the supported page sizes for each GITS_BASERn
register. See the patch below (which I've only boot-tested once on
pretty
standard HW as well as a guest).
This, in turn, simplifies the way we perform the allocation (no nested
retry loop). This is of course a much more patch invasive, but at least
it doesn't leave too much cruft behind.
Please let me know whether this solves your issue.
Thanks,
M.
From 544bf078869fe2baa788b1d3fb487f429144bd01 Mon Sep 17 00:00:00 2001
From: Marc Zyngier <[email protected]>
Date: Fri, 13 Mar 2020 11:01:15 +0000
Subject: [PATCH] irqchip/gic-v3-its: Probe ITS page size for all
GITS_BASERn
registers
The GICv3 ITS driver assumes that once it has latched on a page size for
a given BASER register, it can use the same page size as the maximum
page size for all subsequent BASER registers.
Although it worked so far, nothing in the architecture guarantees this,
and Nianyao Tang hit this problem on some undisclosed implementation.
Let's bite the bullet and probe the the supported page size on all BASER
registers before starting to populate the tables. This simplifies the
setup a bit, at the expense of a few additional MMIO accesses.
Reported-by: Nianyao Tang <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Link:
https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 100 ++++++++++++++++++++-----------
1 file changed, 66 insertions(+), 34 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c
b/drivers/irqchip/irq-gic-v3-its.c
index ee5568c20212..ec8c4df7ebe5 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2222,18 +2222,17 @@ static void its_write_baser(struct its_node
*its, struct its_baser *baser,
}
static int its_setup_baser(struct its_node *its, struct its_baser
*baser,
- u64 cache, u64 shr, u32 psz, u32 order,
- bool indirect)
+ u64 cache, u64 shr, u32 order, bool indirect)
{
u64 val = its_read_baser(its, baser);
u64 esz = GITS_BASER_ENTRY_SIZE(val);
u64 type = GITS_BASER_TYPE(val);
u64 baser_phys, tmp;
- u32 alloc_pages;
+ u32 alloc_pages, psz;
struct page *page;
void *base;
-retry_alloc_baser:
+ psz = baser->psz;
alloc_pages = (PAGE_ORDER_TO_SIZE(order) / psz);
if (alloc_pages > GITS_BASER_PAGES_MAX) {
pr_warn("ITS@%pa: %s too large, reduce ITS pages %u->%u\n",
@@ -2306,25 +2305,6 @@ static int its_setup_baser(struct its_node *its,
struct its_baser *baser,
goto retry_baser;
}
- if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
- /*
- * Page size didn't stick. Let's try a smaller
- * size and retry. If we reach 4K, then
- * something is horribly wrong...
- */
- free_pages((unsigned long)base, order);
- baser->base = NULL;
-
- switch (psz) {
- case SZ_16K:
- psz = SZ_4K;
- goto retry_alloc_baser;
- case SZ_64K:
- psz = SZ_16K;
- goto retry_alloc_baser;
- }
- }
-
if (val != tmp) {
pr_err("ITS@%pa: %s doesn't stick: %llx %llx\n",
&its->phys_base, its_base_type_string[type],
@@ -2350,13 +2330,14 @@ static int its_setup_baser(struct its_node *its,
struct its_baser *baser,
static bool its_parse_indirect_baser(struct its_node *its,
struct its_baser *baser,
- u32 psz, u32 *order, u32 ids)
+ u32 *order, u32 ids)
{
u64 tmp = its_read_baser(its, baser);
u64 type = GITS_BASER_TYPE(tmp);
u64 esz = GITS_BASER_ENTRY_SIZE(tmp);
u64 val = GITS_BASER_InnerShareable | GITS_BASER_RaWaWb;
u32 new_order = *order;
+ u32 psz = baser->psz;
bool indirect = false;
/* No need to enable Indirection if memory requirement < (psz*2)bytes
*/
@@ -2474,11 +2455,58 @@ static void its_free_tables(struct its_node
*its)
}
}
+static int its_probe_baser_psz(struct its_node *its, struct its_baser
*baser)
+{
+ u64 psz = SZ_64K;
+
+ while (psz) {
+ u64 val, gpsz;
+
+ val = its_read_baser(its, baser);
+ val &= ~GITS_BASER_PAGE_SIZE_MASK;
+
+ switch (psz) {
+ case SZ_64K:
+ gpsz = GITS_BASER_PAGE_SIZE_64K;
+ break;
+ case SZ_16K:
+ gpsz = GITS_BASER_PAGE_SIZE_16K;
+ break;
+ case SZ_4K:
+ default:
+ gpsz = GITS_BASER_PAGE_SIZE_4K;
+ break;
+ }
+
+ gpz >>= GITS_BASER_PAGE_SIZE_SHIFT;
+
+ val |= FIELD_PREP(GITS_BASER_PAGE_SIZE_MASK, gpsz);
+ its_write_baser(its, baser, val);
+
+ if (FIELD_GET(GITS_BASER_PAGE_SIZE_MASK, baser->val) == gpsz)
+ break;
+
+ switch (psz) {
+ case SZ_64K:
+ psz = SZ_16K;
+ break;
+ case SZ_16K:
+ psz = SZ_4K;
+ break;
+ case SZ_4K:
+ default:
+ return -1;
+ }
+ }
+
+ baser->psz = psz;
+ return 0;
+}
+
static int its_alloc_tables(struct its_node *its)
{
u64 shr = GITS_BASER_InnerShareable;
u64 cache = GITS_BASER_RaWaWb;
- u32 psz = SZ_64K;
int err, i;
if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_22375)
@@ -2489,16 +2517,22 @@ static int its_alloc_tables(struct its_node
*its)
struct its_baser *baser = its->tables + i;
u64 val = its_read_baser(its, baser);
u64 type = GITS_BASER_TYPE(val);
- u32 order = get_order(psz);
bool indirect = false;
+ u32 order;
- switch (type) {
- case GITS_BASER_TYPE_NONE:
+ if (type == GITS_BASER_TYPE_NONE)
continue;
+ if (its_probe_baser_psz(its, baser)) {
+ its_free_tables(its);
+ return -ENXIO;
+ }
+
+ order = get_order(baser->psz);
+
+ switch (type) {
case GITS_BASER_TYPE_DEVICE:
- indirect = its_parse_indirect_baser(its, baser,
- psz, &order,
+ indirect = its_parse_indirect_baser(its, baser, &order,
device_ids(its));
break;
@@ -2514,20 +2548,18 @@ static int its_alloc_tables(struct its_node
*its)
}
}
- indirect = its_parse_indirect_baser(its, baser,
- psz, &order,
+ indirect = its_parse_indirect_baser(its, baser, &order,
ITS_MAX_VPEID_BITS);
break;
}
- err = its_setup_baser(its, baser, cache, shr, psz, order, indirect);
+ err = its_setup_baser(its, baser, cache, shr, order, indirect);
if (err < 0) {
its_free_tables(its);
return err;
}
/* Update settings which will be used for next BASERn */
- psz = baser->psz;
cache = baser->val & GITS_BASER_CACHEABILITY_MASK;
shr = baser->val & GITS_BASER_SHAREABILITY_MASK;
}
--
2.17.1
--
Jazz is not dead. It just smells funny...
On 2020-03-13 12:00, Marc Zyngier wrote:
[...]
The astute reader will have noticed that I've sent the wrong patch:
> +static int its_probe_baser_psz(struct its_node *its, struct its_baser
> *baser)
> +{
> + u64 psz = SZ_64K;
> +
> + while (psz) {
> + u64 val, gpsz;
> +
> + val = its_read_baser(its, baser);
> + val &= ~GITS_BASER_PAGE_SIZE_MASK;
> +
> + switch (psz) {
> + case SZ_64K:
> + gpsz = GITS_BASER_PAGE_SIZE_64K;
> + break;
> + case SZ_16K:
> + gpsz = GITS_BASER_PAGE_SIZE_16K;
> + break;
> + case SZ_4K:
> + default:
> + gpsz = GITS_BASER_PAGE_SIZE_4K;
> + break;
> + }
> +
> + gpz >>= GITS_BASER_PAGE_SIZE_SHIFT;
s/gpz/gpsz/
Sorry for the noise,
M.
--
Jazz is not dead. It just smells funny...
Hi Shaokun,
On 2020-03-16 05:48, Shaokun Zhang wrote:
> Hi Marc，
>
> On 2020/3/13 20:00, Marc Zyngier wrote:
>> Shaokun, Nianyao,
>>
>> On 2020-03-13 08:46, Shaokun Zhang wrote:
>>> From: Nianyao Tang <[email protected]>
>>>
>>> There is an error when ITS is probed if hw GITS_BASER<2>
>>> only supports psz=SZ_16K while GITS_BASER<1> only supports psz=SZ_4K.
>>> In its_alloc_tables, it has updated GITS_BASER<1>.psz and uses
>>> psz=SZ_4K for setup GITS_BASER<2>, and would fail in writing
>>> GITS_BASER<2>.psz=SZ_4K. 4K PAGE is the smallest and shall stop retry
>>> for other page sizes.
>>>
>>> That latter GITS_BASER<n>.psz is larger than former, will lead
>>> to similar error.
>>>
>>> [ 0.000000] ITS@0x0000000660000000: Virtual CPUs doesn't stick:
>>> ba1f0824404004ff ba1f0824404005ff
>>> [ 0.000000] ITS@0x0000000660000000: failed probing (-6)
>>> [ 0.000000] ITS: No ITS available, not enabling LPIs
>>>
>>> From GIC SPEC document, it's allowed for this implematation, the
>>> latter
>>> GITS_BASER<n>.psz is larger than the former.
>>
>> I was really hoping nobody would build an ITS with disjoint sets of
>> page sizes. Oh well...
>>
>>> Let's fix this error with following patch.
>>>
>>> Cc: Thomas Gleixner <[email protected]>
>>> Cc: Marc Zyngier <[email protected]>
>>> Signed-off-by: Nianyao Tang <[email protected]>
>>> Signed-off-by: Shaokun Zhang <[email protected]>
>>> ---
>>> drivers/irqchip/irq-gic-v3-its.c | 1 -
>>> 1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index 83b1186..59bf8d6 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -2341,7 +2341,6 @@ static int its_alloc_tables(struct its_node
>>> *its)
>>> }
>>>
>>> /* Update settings which will be used for next BASERn */
>>> - psz = baser->psz;
>>> cache = baser->val & GITS_BASER_CACHEABILITY_MASK;
>>> shr = baser->val & GITS_BASER_SHAREABILITY_MASK;
>>> }
>>
>> I think this just papers over the problem, and I'd rather tackle it
>> fully
>> by forcing the probe of the supported page sizes for each GITS_BASERn
>> register. See the patch below (which I've only boot-tested once on
>> pretty
>> standard HW as well as a guest).
>>
>> This, in turn, simplifies the way we perform the allocation (no nested
>> retry loop). This is of course a much more patch invasive, but at
>> least
>> it doesn't leave too much cruft behind.
>>
>> Please let me know whether this solves your issue.
>>
>
> Thanks your quick reply.
> With your following fixed patch, it can work for Nianyao, So:
> Tested-by: Nianyao Tang <[email protected]>
Thanks for the quick feedback. I've now added this patch to the 5.7
queue.
M.
--
Jazz is not dead. It just smells funny...
The following commit has been merged into the irq/core branch of tip:
Commit-ID: d5df9dc96eb7423d3f742b13d5e1e479ff795eaa
Gitweb: https://git.kernel.org/tip/d5df9dc96eb7423d3f742b13d5e1e479ff795eaa
Author: Marc Zyngier <[email protected]>
AuthorDate: Fri, 13 Mar 2020 11:01:15
Committer: Marc Zyngier <[email protected]>
CommitterDate: Mon, 16 Mar 2020 15:48:54
irqchip/gic-v3-its: Probe ITS page size for all GITS_BASERn registers
The GICv3 ITS driver assumes that once it has latched on a page size for
a given BASER register, it can use the same page size as the maximum
page size for all subsequent BASER registers.
Although it worked so far, nothing in the architecture guarantees this,
and Nianyao Tang hit this problem on some undisclosed implementation.
Let's bite the bullet and probe the the supported page size on all BASER
registers before starting to populate the tables. This simplifies the
setup a bit, at the expense of a few additional MMIO accesses.
Signed-off-by: Marc Zyngier <[email protected]>
Reported-by: Nianyao Tang <[email protected]>
Tested-by: Nianyao Tang <[email protected]>
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 100 +++++++++++++++++++-----------
1 file changed, 66 insertions(+), 34 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 6bb2bea..e207fbf 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2036,18 +2036,17 @@ static void its_write_baser(struct its_node *its, struct its_baser *baser,
}
static int its_setup_baser(struct its_node *its, struct its_baser *baser,
- u64 cache, u64 shr, u32 psz, u32 order,
- bool indirect)
+ u64 cache, u64 shr, u32 order, bool indirect)
{
u64 val = its_read_baser(its, baser);
u64 esz = GITS_BASER_ENTRY_SIZE(val);
u64 type = GITS_BASER_TYPE(val);
u64 baser_phys, tmp;
- u32 alloc_pages;
+ u32 alloc_pages, psz;
struct page *page;
void *base;
-retry_alloc_baser:
+ psz = baser->psz;
alloc_pages = (PAGE_ORDER_TO_SIZE(order) / psz);
if (alloc_pages > GITS_BASER_PAGES_MAX) {
pr_warn("ITS@%pa: %s too large, reduce ITS pages %u->%u\n",
@@ -2120,25 +2119,6 @@ retry_baser:
goto retry_baser;
}
- if ((val ^ tmp) & GITS_BASER_PAGE_SIZE_MASK) {
- /*
- * Page size didn't stick. Let's try a smaller
- * size and retry. If we reach 4K, then
- * something is horribly wrong...
- */
- free_pages((unsigned long)base, order);
- baser->base = NULL;
-
- switch (psz) {
- case SZ_16K:
- psz = SZ_4K;
- goto retry_alloc_baser;
- case SZ_64K:
- psz = SZ_16K;
- goto retry_alloc_baser;
- }
- }
-
if (val != tmp) {
pr_err("ITS@%pa: %s doesn't stick: %llx %llx\n",
&its->phys_base, its_base_type_string[type],
@@ -2164,13 +2144,14 @@ retry_baser:
static bool its_parse_indirect_baser(struct its_node *its,
struct its_baser *baser,
- u32 psz, u32 *order, u32 ids)
+ u32 *order, u32 ids)
{
u64 tmp = its_read_baser(its, baser);
u64 type = GITS_BASER_TYPE(tmp);
u64 esz = GITS_BASER_ENTRY_SIZE(tmp);
u64 val = GITS_BASER_InnerShareable | GITS_BASER_RaWaWb;
u32 new_order = *order;
+ u32 psz = baser->psz;
bool indirect = false;
/* No need to enable Indirection if memory requirement < (psz*2)bytes */
@@ -2288,11 +2269,58 @@ static void its_free_tables(struct its_node *its)
}
}
+static int its_probe_baser_psz(struct its_node *its, struct its_baser *baser)
+{
+ u64 psz = SZ_64K;
+
+ while (psz) {
+ u64 val, gpsz;
+
+ val = its_read_baser(its, baser);
+ val &= ~GITS_BASER_PAGE_SIZE_MASK;
+
+ switch (psz) {
+ case SZ_64K:
+ gpsz = GITS_BASER_PAGE_SIZE_64K;
+ break;
+ case SZ_16K:
+ gpsz = GITS_BASER_PAGE_SIZE_16K;
+ break;
+ case SZ_4K:
+ default:
+ gpsz = GITS_BASER_PAGE_SIZE_4K;
+ break;
+ }
+
+ gpsz >>= GITS_BASER_PAGE_SIZE_SHIFT;
+
+ val |= FIELD_PREP(GITS_BASER_PAGE_SIZE_MASK, gpsz);
+ its_write_baser(its, baser, val);
+
+ if (FIELD_GET(GITS_BASER_PAGE_SIZE_MASK, baser->val) == gpsz)
+ break;
+
+ switch (psz) {
+ case SZ_64K:
+ psz = SZ_16K;
+ break;
+ case SZ_16K:
+ psz = SZ_4K;
+ break;
+ case SZ_4K:
+ default:
+ return -1;
+ }
+ }
+
+ baser->psz = psz;
+ return 0;
+}
+
static int its_alloc_tables(struct its_node *its)
{
u64 shr = GITS_BASER_InnerShareable;
u64 cache = GITS_BASER_RaWaWb;
- u32 psz = SZ_64K;
int err, i;
if (its->flags & ITS_FLAGS_WORKAROUND_CAVIUM_22375)
@@ -2303,16 +2331,22 @@ static int its_alloc_tables(struct its_node *its)
struct its_baser *baser = its->tables + i;
u64 val = its_read_baser(its, baser);
u64 type = GITS_BASER_TYPE(val);
- u32 order = get_order(psz);
bool indirect = false;
+ u32 order;
- switch (type) {
- case GITS_BASER_TYPE_NONE:
+ if (type == GITS_BASER_TYPE_NONE)
continue;
+ if (its_probe_baser_psz(its, baser)) {
+ its_free_tables(its);
+ return -ENXIO;
+ }
+
+ order = get_order(baser->psz);
+
+ switch (type) {
case GITS_BASER_TYPE_DEVICE:
- indirect = its_parse_indirect_baser(its, baser,
- psz, &order,
+ indirect = its_parse_indirect_baser(its, baser, &order,
device_ids(its));
break;
@@ -2328,20 +2362,18 @@ static int its_alloc_tables(struct its_node *its)
}
}
- indirect = its_parse_indirect_baser(its, baser,
- psz, &order,
+ indirect = its_parse_indirect_baser(its, baser, &order,
ITS_MAX_VPEID_BITS);
break;
}
- err = its_setup_baser(its, baser, cache, shr, psz, order, indirect);
+ err = its_setup_baser(its, baser, cache, shr, order, indirect);
if (err < 0) {
its_free_tables(its);
return err;
}
/* Update settings which will be used for next BASERn */
- psz = baser->psz;
cache = baser->val & GITS_BASER_CACHEABILITY_MASK;
shr = baser->val & GITS_BASER_SHAREABILITY_MASK;
}