| Message ID | 20240214052050.1966439-1-yoshihiro.shimoda.uh@renesas.com (mailing list archive) |
|---|---|
| State | Under Review |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
| Series | irqchip/gic-v3-its: Workaround Renesas R-Car GICv3 ITS | expand |
[+ 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@renesas.com> 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@tkos.co.il/ 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@renesas.com> > --- > 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.
Hi Marc, Thank you for your review! > From: Marc Zyngier, Sent: Wednesday, February 14, 2024 7:58 PM > > [+ 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@renesas.com> 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 Oops. I'll fix it. > > > > [ 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@tkos.co.il/ > > 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. So, IIUC, I should use DMA API for it instead. > > > > Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> > > --- > > 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. I got it. I'll fix it. > Also: > > - This must carry an official erratum number > > - You must document it in Documentation/arch/arm64/silicon-errata.rst I got it. > > + > > 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. Since some functions doesn't have struct its_node, I think adding the argument makes complex the code. I'll avoid to use a global value somehow. > > > > 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? I'm sorry, I didn't understand that. I'll drop this. > > 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. I'll drop this. > > 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. I'll drop this. > > 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? To be honest, I don't know why, but this is required on my environment. Otherwise, the MSI interrupt never happens. Anyway, I should have learned the hardware bug in detail... > > 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? I'm sorry, I didn't understand the specification. I'll drop this. > > 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. I'll drop this. > > 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? Thank you for the comment. This is not needed on my environment. So, I'll drop it. > > 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. I'll drop this. > > } 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. I'll drop this. > > 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. I understood it. I'll add a condition like its_enable_rk3588001() to affect this quirk for my environment only. I believe that such adding condition is acceptable because the CONFIG_ROCKCHIP_ERRATUM_3588001 is the same iidr value of my environment. > > #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. I'll drop it. > > 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. I'll drop it. > > 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. I'll drop it. > > 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. Thank you very much for your suggestion. I'll learn the architecture spec and reconsider the implementation, especially DMA related. Best regards, Yoshihiro Shimoda > HTH, > > M. > > -- > Without deviation from the norm, progress is not possible.
On Fri, 16 Feb 2024 08:47:04 +0000, Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> wrote: > > Hi Marc, > > Thank you for your review! > > > > @@ -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? > > To be honest, I don't know why, but this is required on my environment. > Otherwise, the MSI interrupt never happens. > Anyway, I should have learned the hardware bug in detail... So the redistributors are also affected by this bug, which makes sense given the monolithic nature of GIC600. This should be handled as a separate allocation constraints (i.e. not using your ITS-specific GFP quirk). [...] > > > @@ -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? > > Thank you for the comment. This is not needed on my environment. > So, I'll drop it. If GICR_PROPBASER needs to be in the low 4GB, it is likely that GICR_PENDBASER has the same constraint. It is just that the GIC600 caches are big enough that evictions are rare, and that you don't see a problem. But it is still very likely to exist. [...] > > 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. > > Thank you very much for your suggestion. I'll learn the architecture spec > and reconsider the implementation, especially DMA related. Using the DMA allocator is an interesting prospect. The only issue with that is that the ITS isn't currently represented as a device, which the DMA allocator requires IIRC. You will either have to plumb something in a low-level allocator, or convert the ITS code to implement a platform device. The latter won't be fun. Thanks, M.
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. + 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; 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); 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); 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); 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)); 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); 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); 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)); 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); } else { - col_map = kcalloc(nr_ites, sizeof(*col_map), GFP_KERNEL); + col_map = kcalloc(nr_ites, sizeof(*col_map), GFP_KERNEL | its_gfp_quirk); 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, + }, #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); 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); 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); if (!its_srat_maps) return;
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: [ 0.000000] Zone ranges: [ 0.000000] DMA [mem 0x0000000048000000-0x00000000ffffffff] [ 0.000000] DMA32 empty [ 0.000000] Normal [mem 0x0000000100000000-0x00000004ffffffff] Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com> --- arch/arm64/Kconfig | 9 +++++ drivers/irqchip/irq-gic-v3-its.c | 59 +++++++++++++++++++++----------- 2 files changed, 48 insertions(+), 20 deletions(-)