Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] irqchip/gic-v3-its: order mismatch in allocate_vpe_l1_table() on ITS inherit - [Medium] irqchip/gic-v3-its: memory leak of vpe_table_mask in allocate_vpe_l1_table() --
commit d29e0b57c45efd854666ff7af1f84a27011e992c Author: Aneesh Kumar K.V (Arm) <[email protected]> irqchip/gic-v3-its: Preallocate VPE L1 tables This commit removes the GFP_ATOMIC VPE L1 table page allocation from the CPU-starting callback by using a blocking allocation on the boot CPU, recording the table order, and preallocating pages for secondary CPUs in the sleepable CPU hotplug prepare stage. > diff --git a/drivers/irqchip/irq-gic-v3-its.c > b/drivers/irqchip/irq-gic-v3-its.c > index 9ad66253ab166..5628f497d389d 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c [ ... ] > @@ -2990,7 +3014,16 @@ static int allocate_vpe_l1_table(void) > > pr_debug("np = %d, npg = %lld, psz = %d, epp = %d, esz = %d\n", > np, npg, psz, epp, esz); > - page = its_alloc_pages(GFP_ATOMIC | __GFP_ZERO, get_order(np * > PAGE_SIZE)); > + order = get_order(np * PAGE_SIZE); > + if (use_prealloc) { > + if (WARN_ON_ONCE(order != vpe_l1_prealloc_order)) > + return -EINVAL; > + > + page = gic_data_rdist()->vpe_l1_prealloc; > + gic_data_rdist()->vpe_l1_prealloc = NULL; > + } else { > + page = its_alloc_pages(GFP_KERNEL | __GFP_ZERO, order); > + } > if (!page) > return -ENOMEM; [Severity: Medium] Does this leak gic_data_rdist()->vpe_table_mask on these error returns? Earlier in allocate_vpe_l1_table(), when inherit_vpe_l1_table_from_rd() does not find an existing table to inherit, vpe_table_mask is allocated: gic_data_rdist()->vpe_table_mask = kzalloc_obj(cpumask_t, GFP_ATOMIC); if (!gic_data_rdist()->vpe_table_mask) return -ENOMEM; If inherit_vpe_l1_table_from_its() also does not find a table to inherit and allocate_vpe_l1_table() subsequently fails because order != vpe_l1_prealloc_order or page is NULL, it returns -EINVAL or -ENOMEM without freeing gic_data_rdist()->vpe_table_mask or clearing the pointer. > @@ -3007,6 +3040,9 @@ static int allocate_vpe_l1_table(void) > val |= GICR_VPROPBASER_4_1_VALID; > > out: > + if (!use_prealloc) > + vpe_l1_prealloc_order = vpe_l1_table_order(val); [Severity: High] Can vpe_l1_prealloc_order mismatch the order computed for a secondary CPU when the boot CPU inherits its VPE table from an ITS? On the boot CPU, gic_init_bases() -> its_cpu_init(false) -> its_cpu_init_lpis(false) -> allocate_vpe_l1_table(false) calls inherit_vpe_l1_table_from_its(): val = inherit_vpe_l1_table_from_its(); if (val & GICR_VPROPBASER_4_1_VALID) goto out; When the boot CPU redistributor shares a VPE table with an ITS in its affinity domain, this jumps to out: without probing GICR_VPROPBASER, and vpe_l1_prealloc_order is computed from the inherited GITS_BASER2 value. However, the ITS VPE table is sized by its_parse_indirect_baser(), which only switches to a two-level indirect table when the flat table exceeds two GIC pages and GITS_BASER_INDIRECT is supported: /* No need to enable Indirection if memory requirement < (psz*2)bytes */ if ((esz << ids) > (psz * 2)) { By contrast, when a secondary CPU in another affinity domain cannot inherit from a redistributor or an ITS, allocate_vpe_l1_table(true) probes GICR_VPROPBASER on the redistributor and enables indirection whenever the flat table exceeds a single GIC page: if (epp < ITS_MAX_VPEID) { int nl2; val |= GICR_VPROPBASER_4_1_INDIRECT; For example, when the flat table is 2 * psz (or when GITS_BASER2 uses a flat table or a different probed page size than GICR_VPROPBASER), the inherited ITS table order is get_order(2 * psz) while the secondary redistributor L1 table order is get_order(psz). When that secondary CPU comes online, wouldn't allocate_vpe_l1_table(true) hit WARN_ON_ONCE(order != vpe_l1_prealloc_order), return -EINVAL, and cause its_cpu_init_lpis() to disable GICv4 support? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=14
