Messages in this thread | | | Date | Tue, 19 Jul 2022 09:33:33 +0100 | From | Marc Zyngier <> | Subject | Re: [PATCH] drivers: irqchip: Allocate alignment addr by ITS_BASER.Page_size |
| |
On Mon, 18 Jul 2022 08:35:47 +0100, wangwudi <wangwudi@hisilicon.com> wrote: > > Hi Marc, > > 在 2022/7/16 17:30, Marc Zyngier 写道: > > On Sat, 16 Jul 2022 08:05:36 +0100, > > wangwudi <wangwudi@hisilicon.com> wrote: > >> > >> The description of the ITS_BASER.Physical_Address field in the ARM GIC spec is as > >> follows: > >> "The address must be aligned to the size specified in the Page Size field." > >> The Page_Size field in ITS_BASER might be RO. > >> > >> Currently, the address is aligned based on the system page_size, not the HW > >> Page_Size field. In some case, this is in contradiction with the spec. > >> > >> For example: > >> ITS_BASER.Page_Size indicate 16K, and kernel page size is 4K. > >> If HW need 4K-size memory, the driver may alloc a 4K aligned address. > >> This has been proven in hardware. > > > > Ah, interesting bug. Thanks for bringing this up. Can you describe how > > this occurs? I suspect you are using indirect tables. > > > > Sure. In the system, kernel page size is 4K, and ITS_BASER.Page_Size is 16K. > > As you suspected, HW used indirect VPE table and indication supports a small > number of vpeids, like 2-bits vpeid. So that HW requires only less than 4K- > size memory, and 16K aligned base address. But driver alloctes 4K aligend base > address.
Well, that I dispute, see below.
> > >> > >> Cc: Thomas Gleixner <tglx@linutronix.de> > >> Cc: Marc Zyngier <maz@kernel.org> > >> Signed-off-by: wangwudi <wangwudi@hisilicon.com> > >> --- > >> drivers/irqchip/irq-gic-v3-its.c | 3 +++ > >> 1 file changed, 3 insertions(+) > >> > >> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > >> index 5ff09de6c48f..0e25e887d45c 100644 > >> --- a/drivers/irqchip/irq-gic-v3-its.c > >> +++ b/drivers/irqchip/irq-gic-v3-its.c > >> @@ -2310,6 +2310,9 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > >> order = get_order(GITS_BASER_PAGES_MAX * psz); > >> } > >> > >> + if ((psz > PAGE_SIZE) && (PAGE_ORDER_TO_SIZE(order) < psz)) { > >> + order = get_order(psz); > >> + } > >> page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO, order); > >> if (!page) > >> return -ENOMEM; > > > > However, I don't see how you end-up with the incorrect value here. > > > > * No indirect table: > > alloc_its_tables(): > > order = get_order(baser->psz); > > > > * Indirect tables: > > alloc_its_tables(): > > order = get_order(baser->psz); > > its_parse_indirect_baser(): > > new_order = *order; > > new_order = max_t(u32, get_order(esz << ids), new_order); > > > > So in both cases, we should end-up with order >= get_order(psz). > Yes, totally agree.
OK. So what does your patch actually fixes?
> > > > > Clearly, I'm missing a path, but your commit message doesn't make it > > obvious. Can you please enlighten me? > > > My commit is based on the premise: > "alloc_pages_node gives a size-aligend address". > For example, if HW apply for 4K-size memory, then allocated address is 4K > aligned.
Right. And if baser->psz is 16K, the memory returned will be 16K aligned.
The only thing I can imagine is that there is a code path that doesn't use baser->psz as the minimum value when allocating memory programmed into a ITS_BASER register. But I can't see that path. Can you?
M.
-- Without deviation from the norm, progress is not possible.
| |