lkml.org 
[lkml]   [2020]   [Feb]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH RFC 3/5] irqchip/gic-v4.1: Ensure L2 vPE table is allocated at RD level
Hi Zenghui,

Thanks for this. A few comments below.

On 2020-02-04 09:09, Zenghui Yu wrote:
> In GICv4, we will ensure that level2 vPE table memory is allocated
> for the specified vpe_id on all v4 ITS, in its_alloc_vpe_table().
> This still works well for the typical GICv4.1 implementation, where
> the new vPE table is shared between the ITSs and the RDs.
>
> To make things explicit, introduce allocate_vpe_l1_table_entry() to
> make sure that the L2 tables are allocated on all v4.1 RDs. We're
> likely not need to allocate memory in it because the vPE table is
> shared and (L2 table is) already allocated at ITS level, except
> for the case where the ITS doesn't share anything (say SVPET == 0,
> practically unlikely but architecturally allowed).

Huh... Interesting. I definitely don't anticipate the case to pop up,
but you are right, this is architecturally allowed.

> The implementation of allocate_vpe_l1_table_entry() is mostly
> copied from its_alloc_table_entry().
>
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 68 ++++++++++++++++++++++++++++++++
> 1 file changed, 68 insertions(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index 0f1fe56ce0af..c1d01454eec8 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2443,6 +2443,64 @@ static u64
> inherit_vpe_l1_table_from_rd(cpumask_t **mask)
> return 0;
> }
>
> +static int allocate_vpe_l1_table_entry(int cpu, u32 id)

Given that this actually allocates the L2 table, I'd rather have it
called
something like allocate_vpe_l2_table() as the pendant of
allocate_vpe_l1_table().

This should also properly return a bool rather then an int.

> +{
> + void __iomem *base = gic_data_rdist_cpu(cpu)->rd_base;
> + u64 val, gpsz, npg;
> + unsigned int psz, esz, idx;
> + struct page *page;
> + __le64 *table;
> +
> + if (!gic_rdists->has_rvpeid)
> + return true;
> +
> + val = gits_read_vpropbaser(base + SZ_128K + GICR_VPROPBASER);
> +
> + esz = FIELD_GET(GICR_VPROPBASER_4_1_ENTRY_SIZE, val) + 1;
> + gpsz = FIELD_GET(GICR_VPROPBASER_4_1_PAGE_SIZE, val);
> + npg = FIELD_GET(GICR_VPROPBASER_4_1_SIZE, val) + 1;
> +
> + switch (gpsz) {
> + default:
> + WARN_ON(1);
> + /* fall through */
> + case GIC_PAGE_SIZE_4K:
> + psz = SZ_4K;
> + break;
> + case GIC_PAGE_SIZE_16K:
> + psz = SZ_16K;
> + break;
> + case GIC_PAGE_SIZE_64K:
> + psz = SZ_64K;
> + break;
> + }
> +
> + /* Don't allow vpe_id that exceeds single, flat table limit */
> + if (!(val & GICR_VPROPBASER_4_1_INDIRECT))
> + return (id < (npg * psz / (esz * SZ_8)));
> +
> + /* Compute 1st level table index & check if that exceeds table limit
> */
> + idx = id >> ilog2(psz / (esz * SZ_8));
> + if (idx >= (npg * psz / GITS_LVL1_ENTRY_SIZE))
> + return false;
> +
> + table = gic_data_rdist_cpu(cpu)->vpe_l1_base;
> +
> + /* Allocate memory for 2nd level table */
> + if (!table[idx]) {
> + page = alloc_pages(GFP_KERNEL | __GFP_ZERO, get_order(psz));
> + if (!page)
> + return false;
> +
> + table[idx] = cpu_to_le64(page_to_phys(page) | GITS_BASER_VALID);

I think we may need a cache flushing here in some circumstances. We
certainly
have it in its_alloc_table_entry.

> +
> + /* Ensure updated table contents are visible to RD hardware */
> + dsb(sy);
> + }
> +
> + return true;
> +}
> +
> static int allocate_vpe_l1_table(void)
> {
> void __iomem *vlpi_base = gic_data_rdist_vlpi_base();
> @@ -2957,6 +3015,7 @@ static bool its_alloc_device_table(struct
> its_node *its, u32 dev_id)
> static bool its_alloc_vpe_table(u32 vpe_id)
> {
> struct its_node *its;
> + int cpu;
>
> /*
> * Make sure the L2 tables are allocated on *all* v4 ITSs. We
> @@ -2979,6 +3038,15 @@ static bool its_alloc_vpe_table(u32 vpe_id)
> return false;
> }
>
> + /*
> + * Make sure the L2 tables are allocated for all copies of
> + * the L1 table on *all* v4.1 RDs.
> + */
> + for_each_possible_cpu(cpu) {

not: You could predicate this on gic_rdists->has_rvpeid so that you
don't
iterate pointlessly on non v4.1 HW.

> + if (!allocate_vpe_l1_table_entry(cpu, vpe_id))
> + return false;
> + }
> +
> return true;
> }

Otherwise, looks good to me.

M.
--
Jazz is not dead. It just smells funny...

\
 
 \ /
  Last update: 2020-02-05 13:27    [W:0.089 / U:0.172 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site