irqchip/gic-v3-its:Fix the bug while calculating the page number of ITS table
Subsystems (2)
RFC PATCH 1/1 irqchip/gic-v3-its:Fix the bug while calculating the page number of ITS table
From: Ma Jun <[email protected]>
Hi Marc, Robert:
Maybe there is a bug introduced by commit "irqchip/gicv3-its: Add range check for number of allocated pages" 30f2136346cab91e1ffd9ee6370d76809f20487a
I think, before setting the page number, the variable "alloc_pages" should be calculated and checked again.
Or else, the page number programmed into GITS_BASER register is always the number of 64KB even though the page size is 16KB or 4KB.
--- drivers/irqchip/irq-gic-v3-its.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
drivers/irqchip/irq-gic-v3-its.c +8 -1
@@ -879,7 +879,7 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)if (alloc_pages >GITS_BASER_PAGES_MAX ) { alloc_pages =GITS_BASER_PAGES_MAX ; order =get_order (GITS_BASER_PAGES_MAX * psz); -pr_warn ("%s: Device Table too large, reduce its page order to %u (%u pages)\n" , +pr_warn ("%s: Table too large, reduce its page order to %u (%u pages)\n" , node_name, order, alloc_pages); }
@@ -911,6 +911,13 @@ retry_baser:break ; } + alloc_pages = (alloc_size / psz); +if (alloc_pages >GITS_BASER_PAGES_MAX ) { + alloc_pages =GITS_BASER_PAGES_MAX ; +pr_warn ("%s: Table too large, reduce its page number to %u pages\n" , + node_name, alloc_pages); + } + val |= alloc_pages -1 ;writeq_relaxed (val, its->base +GITS_BASER + i *8 );
Signature · 1 line
1.7.1
RFC PATCH 1/1 irqchip/gic-v3-its:Fix the bug while calculating the page number of ITS table
Quoted 23 lines · On Tue, 22 Dec 2015 15:10:23 +0800 MaJun <[email protected]> wrote:
From: Ma Jun <[email protected]>
Hi Marc, Robert:
Maybe there is a bug introduced by commit "irqchip/gicv3-its: Add range check for number of allocated pages"
30f2136346cab91e1ffd9ee6370d76809f20487aI think, before setting the page number, the variable "
alloc_pages" should be calculated and checked again.
Or else, the page number programmed intoGITS_BASERregister is always the number of 64KB even though the page size is 16KB or 4KB.Signed-off-by Ma Jun <[email protected]>--- drivers/irqchip/irq-gic-v3-its.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)drivers/irqchip/irq-gic-v3-its.c +0
-0
drivers/irqchip/irq-gic-v3-its.c static int its_alloc_tables(const char *node_name, struct its_node *its) · line 883
Quoted 6 lines
drivers/irqchip/irq-gic-v3-its.c +1
-1@@ -879,7 +879,7 @@ static int its_alloc_tables(const char *node_name, struct its_node *its)if (alloc_pages >GITS_BASER_PAGES_MAX ) { alloc_pages =GITS_BASER_PAGES_MAX ; order =get_order (GITS_BASER_PAGES_MAX * psz); -pr_warn ("%s: Device Table too large, reduce its page order to %u (%u pages)\n" , +pr_warn ("%s: Table too large, reduce its page order to %u (%u pages)\n" , node_name, order, alloc_pages);
No, this can only be a device table. Even with the smallest ITS page size, you cannot end-up with with more than 256 ITS pages, since the original allocation is still a single CPU page.
drivers/irqchip/irq-gic-v3-its.c retry_baser: · line 920
Quoted 13 lines
drivers/irqchip/irq-gic-v3-its.c +7
-0}@@ -911,6 +911,13 @@ retry_baser:break ; } + alloc_pages = (alloc_size / psz); +if (alloc_pages >GITS_BASER_PAGES_MAX ) { + alloc_pages =GITS_BASER_PAGES_MAX ; +pr_warn ("%s: Table too large, reduce its page number to %u pages\n" , + node_name, alloc_pages); + } +
So you now have limited the number of pages, but you also have allocated too much memory. Not really ideal.
drivers/irqchip/irq-gic-v3-its.c retry_baser: · line 923
Quoted 3 lines
drivers/irqchip/irq-gic-v3-its.c +0
-0val |= alloc_pages -1 ;writeq_relaxed (val, its->base +GITS_BASER + i *8 );
Instead of duplicating that code and making it even less readable, how about something like this, which simply goes back to a clean slate and tries it again with the new page size:
drivers/irqchip/irq-gic-v3-its.c +6 -2
@@ -875,6 +875,7 @@ static int its_alloc_tables(const char *node_name, struct its_node *its) } alloc_size = (1 << order) *PAGE_SIZE ; +retry_alloc_baser: alloc_pages = (alloc_size / psz);if (alloc_pages >GITS_BASER_PAGES_MAX ) { alloc_pages =GITS_BASER_PAGES_MAX ;
@@ -938,13 +939,16 @@ retry_baser:* size and retry. If we reach 4K, then * something is horribly wrong... */ +free_pages ((unsigned long )base, order); + its->tables[i] =NULL ; +switch (psz) {case SZ_16K : psz =SZ_4K ; - goto retry_baser; + goto retry_alloc_baser;case SZ_64K : psz =SZ_16K ; - goto retry_baser; + goto retry_alloc_baser; } }
I haven't tested it, but it looks less invasive. Care to give it a go?
Thanks,
M.
Signature · 1 line
Jazz is not dead. It just smells funny.
RFC PATCH 1/1 irqchip/gic-v3-its:Fix the bug while calculating the page number of ITS table
Hi Marc:
? 2015/12/24 21:48, Marc Zyngier ??:
Quoted 3 lines
On Tue, 22 Dec 2015 15:10:23 +0800
MaJun <[email protected]> wrote:
[...]
Quoted 34 lines
drivers/irqchip/irq-gic-v3-its.c +6
-2@@ -875,6 +875,7 @@ static int its_alloc_tables(const char *node_name, struct its_node *its) } alloc_size = (1 << order) *PAGE_SIZE ; +retry_alloc_baser: alloc_pages = (alloc_size / psz);if (alloc_pages >GITS_BASER_PAGES_MAX ) { alloc_pages =GITS_BASER_PAGES_MAX ;@@ -938,13 +939,16 @@ retry_baser:* size and retry. If we reach 4K, then * something is horribly wrong... */ +free_pages ((unsigned long )base, order); + its->tables[i] =NULL ; +switch (psz) {case SZ_16K : psz =SZ_4K ; - goto retry_baser; + goto retry_alloc_baser;case SZ_64K : psz =SZ_16K ; - goto retry_baser; + goto retry_alloc_baser; } }
Quoted 2 lines
I haven't tested it, but it looks less invasive. Care to give it a go?
This patch works fine on my board.
Thanks
Ma Jun
Quoted 4 lines
Thanks,
M.