[+ Geert, who deals with Renesas SoCs on a daily basis] Hi Yoshihiro, On Wed, 14 Feb 2024 05:20:50 +0000, Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> wrote: > > The GICv3 ITS on Renesas R-Car has limitations: > - It can access lower 32-bits memory area only. > - It cannot use Sharable/cache attributes. > > So, set ITS_FLAGS_FORCE_NON_SHAREABLE flag, and set GFP_DMA to all > memory allocation APIs for getting lower 32-bits memory area on > the R-Car. Note that GFP_DMA32 cannot work correctly because > the environment doens't have DMA32 zone like below: nit: doesn't > > [ 0.000000] Zone ranges: > [ 0.000000] DMA [mem 0x0000000048000000-0x00000000ffffffff] > [ 0.000000] DMA32 empty > [ 0.000000] Normal [mem 0x0000000100000000-0x00000004ffffffff] Unfortunately, none of that is a universal truth. The way DMA and DMA32 are expressed is pretty variable. See this for example: https://lore.kernel.org/all/cover.1703683642.git.baruch@xxxxxxxxxx/ So I can't see how you can reliably rely on this layout. It may work today on your platform, but I wouldn't take this as a guarantee. > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@xxxxxxxxxxx> > --- > arch/arm64/Kconfig | 9 +++++ > drivers/irqchip/irq-gic-v3-its.c | 59 +++++++++++++++++++++----------- > 2 files changed, 48 insertions(+), 20 deletions(-) > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index c1a16a9dae72..49fe3006e142 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -1250,6 +1250,15 @@ config SOCIONEXT_SYNQUACER_PREITS > > If unsure, say Y. > > +config RENESAS_RCAR_GIC_ITS > + bool "Renesas R-Car: Workaround for GICv3 ITS" > + default n > + help > + Renesas R-Car Gen4 SoCs has a limitation about GICv3 ITS which can > + access lower 32-bits memory address range only. > + > + If unsure, say Y. Either this should be 'default y', or you should drop this last line. Being 'default n' and saying 'if you don't know, say y' is counterproductive. Also: - This must carry an official erratum number - You must document it in Documentation/arch/arm64/silicon-errata.rst > + > endmenu # "ARM errata workarounds via the alternatives framework" > > choice > diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c > index d097001c1e3e..e0e672b561b9 100644 > --- a/drivers/irqchip/irq-gic-v3-its.c > +++ b/drivers/irqchip/irq-gic-v3-its.c > @@ -186,6 +186,7 @@ static LIST_HEAD(its_nodes); > static DEFINE_RAW_SPINLOCK(its_lock); > static struct rdists *gic_rdists; > static struct irq_domain *its_parent; > +static gfp_t its_gfp_quirk; Why is that global? Why isn't this computed on a per-ITS basis? Since you are introducing a generic mechanism here, you shouldn't assume that all ITSs are subjected to the same issue. > > static unsigned long its_list_map; > static u16 vmovp_seq_num; > @@ -1846,7 +1847,7 @@ static int its_vlpi_map(struct irq_data *d, struct its_cmd_info *info) > struct its_vlpi_map *maps; > > maps = kcalloc(its_dev->event_map.nr_lpis, sizeof(*maps), > - GFP_ATOMIC); > + GFP_ATOMIC | its_gfp_quirk); This data structure is never exposed to the GIC. Why do we need to constraint this allocation? > if (!maps) { > ret = -ENOMEM; > goto out; > @@ -2044,7 +2045,7 @@ static struct lpi_range *mk_lpi_range(u32 base, u32 span) > { > struct lpi_range *range; > > - range = kmalloc(sizeof(*range), GFP_KERNEL); > + range = kmalloc(sizeof(*range), GFP_KERNEL | its_gfp_quirk); Same thing. > if (range) { > range->base_id = base; > range->span = span; > @@ -2169,7 +2170,7 @@ static unsigned long *its_lpi_alloc(int nr_irqs, u32 *base, int *nr_ids) > if (err) > goto out; > > - bitmap = bitmap_zalloc(nr_irqs, GFP_ATOMIC); > + bitmap = bitmap_zalloc(nr_irqs, GFP_ATOMIC | its_gfp_quirk); Same thing. > if (!bitmap) > goto out; > > @@ -2201,7 +2202,7 @@ static struct page *its_allocate_prop_table(gfp_t gfp_flags) > { > struct page *prop_page; > > - prop_page = alloc_pages(gfp_flags, get_order(LPI_PROPBASE_SZ)); > + prop_page = alloc_pages(gfp_flags | its_gfp_quirk, get_order(LPI_PROPBASE_SZ)); This only affects the redistributors. Why should we constraint it? > if (!prop_page) > return NULL; > > @@ -2335,7 +2336,7 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser, > order = get_order(GITS_BASER_PAGES_MAX * psz); > } > > - page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO, order); > + page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO | its_gfp_quirk, order); > if (!page) > return -ENOMEM; > > @@ -2808,7 +2809,7 @@ static bool allocate_vpe_l2_table(int cpu, u32 id) > > /* Allocate memory for 2nd level table */ > if (!table[idx]) { > - page = alloc_pages(GFP_KERNEL | __GFP_ZERO, get_order(psz)); > + page = alloc_pages(GFP_KERNEL | __GFP_ZERO | its_gfp_quirk, get_order(psz)); > if (!page) > return false; > > @@ -2860,7 +2861,7 @@ static int allocate_vpe_l1_table(void) > if (val & GICR_VPROPBASER_4_1_VALID) > goto out; > > - gic_data_rdist()->vpe_table_mask = kzalloc(sizeof(cpumask_t), GFP_ATOMIC); > + gic_data_rdist()->vpe_table_mask = kzalloc(sizeof(cpumask_t), GFP_ATOMIC | its_gfp_quirk); This is a cpumask allocation, not exposed to the GIC. What is the reason for constraining it? > if (!gic_data_rdist()->vpe_table_mask) > return -ENOMEM; > > @@ -2927,7 +2928,7 @@ 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 = alloc_pages(GFP_ATOMIC | __GFP_ZERO, get_order(np * PAGE_SIZE)); > + page = alloc_pages(GFP_ATOMIC | __GFP_ZERO | its_gfp_quirk, get_order(np * PAGE_SIZE)); > if (!page) > return -ENOMEM; > > @@ -2957,7 +2958,7 @@ static int its_alloc_collections(struct its_node *its) > int i; > > its->collections = kcalloc(nr_cpu_ids, sizeof(*its->collections), > - GFP_KERNEL); > + GFP_KERNEL | its_gfp_quirk); Same thing. > if (!its->collections) > return -ENOMEM; > > @@ -2971,7 +2972,7 @@ static struct page *its_allocate_pending_table(gfp_t gfp_flags) > { > struct page *pend_page; > > - pend_page = alloc_pages(gfp_flags | __GFP_ZERO, > + pend_page = alloc_pages(gfp_flags | __GFP_ZERO | its_gfp_quirk, > get_order(LPI_PENDBASE_SZ)); This is a redistributor-private allocation. If it is the ITSs that are affected by this bug, why are the pending tables constrained? > if (!pend_page) > return NULL; > @@ -3319,7 +3320,7 @@ static bool its_alloc_table_entry(struct its_node *its, > > /* Allocate memory for 2nd level table */ > if (!table[idx]) { > - page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO, > + page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO | its_gfp_quirk, > get_order(baser->psz)); > if (!page) > return false; > @@ -3415,7 +3416,7 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id, > if (WARN_ON(!is_power_of_2(nvecs))) > nvecs = roundup_pow_of_two(nvecs); > > - dev = kzalloc(sizeof(*dev), GFP_KERNEL); > + dev = kzalloc(sizeof(*dev), GFP_KERNEL | its_gfp_quirk); > /* > * Even if the device wants a single LPI, the ITT must be > * sized as a power of two (and you need at least one bit...). > @@ -3423,14 +3424,14 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id, > nr_ites = max(2, nvecs); > sz = nr_ites * (FIELD_GET(GITS_TYPER_ITT_ENTRY_SIZE, its->typer) + 1); > sz = max(sz, ITS_ITT_ALIGN) + ITS_ITT_ALIGN - 1; > - itt = kzalloc_node(sz, GFP_KERNEL, its->numa_node); > + itt = kzalloc_node(sz, GFP_KERNEL | its_gfp_quirk, its->numa_node); > if (alloc_lpis) { > lpi_map = its_lpi_alloc(nvecs, &lpi_base, &nr_lpis); > if (lpi_map) > col_map = kcalloc(nr_lpis, sizeof(*col_map), > - GFP_KERNEL); > + GFP_KERNEL | its_gfp_quirk); This is kernel-private memory, not visible by the GIC. > } else { > - col_map = kcalloc(nr_ites, sizeof(*col_map), GFP_KERNEL); > + col_map = kcalloc(nr_ites, sizeof(*col_map), GFP_KERNEL | its_gfp_quirk); Same thing. > nr_lpis = 0; > lpi_base = 0; > } > @@ -4754,6 +4755,16 @@ static bool __maybe_unused its_enable_rk3588001(void *data) > return true; > } > > +static bool __maybe_unused its_enable_renesas_rcar_gic_its(void *data) > +{ > + struct its_node *its = data; > + > + its->flags |= ITS_FLAGS_FORCE_NON_SHAREABLE; > + its_gfp_quirk = GFP_DMA; > + > + return true; > +} > + > static bool its_set_non_coherent(void *data) > { > struct its_node *its = data; > @@ -4815,6 +4826,14 @@ static const struct gic_quirk its_quirks[] = { > .mask = 0xffffffff, > .init = its_enable_rk3588001, > }, > +#endif > +#ifdef CONFIG_RENESAS_RCAR_GIC_ITS > + { > + .desc = "ITS: Renesas R-Car Gen4 GICv3 ITS", > + .iidr = 0x0201743b, > + .mask = 0xffffffff, > + .init = its_enable_renesas_rcar_gic_its, > + }, Huh. This describes a GIC600 implemented by ARM. Which means your are actively forcing your quirk on *every* implementations, including those that are not affected by this hardware issue. Definitely not acceptable. > #endif > { > .desc = "ITS: non-coherent attribute", > @@ -4974,7 +4993,7 @@ static int its_init_domain(struct its_node *its) > struct irq_domain *inner_domain; > struct msi_domain_info *info; > > - info = kzalloc(sizeof(*info), GFP_KERNEL); > + info = kzalloc(sizeof(*info), GFP_KERNEL | its_gfp_quirk); Kernel memory only. > if (!info) > return -ENOMEM; > > @@ -5011,7 +5030,7 @@ static int its_init_vpe_domain(void) > > entries = roundup_pow_of_two(nr_cpu_ids); > vpe_proxy.vpes = kcalloc(entries, sizeof(*vpe_proxy.vpes), > - GFP_KERNEL); > + GFP_KERNEL | its_gfp_quirk); > if (!vpe_proxy.vpes) > return -ENOMEM; > > @@ -5108,7 +5127,7 @@ static int __init its_probe_one(struct its_node *its) > } > } > > - page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO, > + page = alloc_pages_node(its->numa_node, GFP_KERNEL | __GFP_ZERO | its_gfp_quirk, > get_order(ITS_CMD_QUEUE_SZ)); > if (!page) { > err = -ENOMEM; > @@ -5352,7 +5371,7 @@ static struct its_node __init *its_node_init(struct resource *res, > > pr_info("ITS %pR\n", res); > > - its = kzalloc(sizeof(*its), GFP_KERNEL); > + its = kzalloc(sizeof(*its), GFP_KERNEL | its_gfp_quirk); Kernel memory only. > if (!its) > goto out_unmap; > > @@ -5520,7 +5539,7 @@ static void __init acpi_table_parse_srat_its(void) > return; > > its_srat_maps = kmalloc_array(count, sizeof(struct its_srat_map), > - GFP_KERNEL); > + GFP_KERNEL | its_gfp_quirk); Kernel memory only. > if (!its_srat_maps) > return; > I suggest that you start reading the architecture spec and understand what is the memory that is accessible by the GIC, because at least half of this patch is completely unnecessary. You also need to be clear about what is affected by this bug: ITS only? or also the RDs? If both are affected, they need to be handled separately. In any case, you must not assume that all ITSs in a system are subjected to this bug, which means that you must have per-ITS tracking of the GFP flags. Finally, the DMA story itself needs to be sorted out, because you are relying on something that is incredibly fragile. HTH, M. -- Without deviation from the norm, progress is not possible.
