lkml.org 
[lkml]   [2020]   [Feb]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v4 08/20] irqchip/gic-v4.1: Plumb get/set_irqchip_state SGI callbacks
Hi Zenghui,

On 2020-02-18 07:00, Zenghui Yu wrote:
> Hi Marc,
>
> On 2020/2/14 22:57, Marc Zyngier wrote:
>> To implement the get/set_irqchip_state callbacks (limited to the
>> PENDING state), we have to use a particular set of hacks:
>>
>> - Reading the pending state is done by using a pair of new
>> redistributor
>> registers (GICR_VSGIR, GICR_VSGIPENDR), which allow the 16
>> interrupts
>> state to be retrieved.
>> - Setting the pending state is done by generating it as we'd otherwise
>> do
>> for a guest (writing to GITS_SGIR)
>> - Clearing the pending state is done by emiting a VSGI command with
>> the
>> "clear" bit set.
>>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 56
>> ++++++++++++++++++++++++++++++
>> include/linux/irqchip/arm-gic-v3.h | 14 ++++++++
>> 2 files changed, 70 insertions(+)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 1e448d9a16ea..a9753435c4ff 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -3915,11 +3915,67 @@ static int its_sgi_set_affinity(struct
>> irq_data *d,
>> return -EINVAL;
>> }
>> +static int its_sgi_set_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which,
>> + bool state)
>> +{
>> + if (which != IRQCHIP_STATE_PENDING)
>> + return -EINVAL;
>> +
>> + if (state) {
>> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
>> + struct its_node *its = find_4_1_its();
>> + u64 val;
>> +
>> + val = FIELD_PREP(GITS_SGIR_VPEID, vpe->vpe_id);
>> + val |= FIELD_PREP(GITS_SGIR_VINTID, d->hwirq);
>> + writeq_relaxed(val, its->sgir_base + GITS_SGIR - SZ_128K);
>> + } else {
>> + its_configure_sgi(d, true);
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int its_sgi_get_irqchip_state(struct irq_data *d,
>> + enum irqchip_irq_state which, bool *val)
>> +{
>> + struct its_vpe *vpe = irq_data_get_irq_chip_data(d);
>> + void __iomem *base = gic_data_rdist_cpu(vpe->col_idx)->rd_base +
>> SZ_128K;
>
> There might be a race on reading the 'vpe->col_idx' against a
> concurrent
> vPE schedule (col_idx will be modified in its_vpe_set_affinity)? Will
> we
> end up accessing the GICR_VSGI* registers of the old redistributor,
> while the vPE is now resident on the new one? Or is it harmful?

Very well spotted. There is a potential problem if old and new RDs are
not part
of the same CommonLPIAff group.

> The same question for direct_lpi_inv(), where 'vpe->col_idx' will be
> used in irq_to_cpuid().

Same problem indeed. We need to ensure that no VMOVP operation can occur
whilst
we use col_idx to access a redistributor. This means a vPE lock of some
sort
that will protect the affinity.

But I think there is a slightly more general problem here, which we
failed to
see initially: the same issue exists for physical LPIs, as col_map[] can
be
updated (its_set_affinity()) in parallel with a direct invalidate.

The good old invalidation through the ITS does guarantee that the two
operation
don't overlap, but direct invalidation breaks it.

Let me have a think about it.

>
>> + u32 count = 1000000; /* 1s! */
>> + u32 status;
>> +
>> + if (which != IRQCHIP_STATE_PENDING)
>> + return -EINVAL;
>> +
>> + writel_relaxed(vpe->vpe_id, base + GICR_VSGIR);
>> + do {
>> + status = readl_relaxed(base + GICR_VSGIPENDR);
>> + if (!(status & GICR_VSGIPENDR_BUSY))
>> + goto out;
>> +
>> + count--;
>> + if (!count) {
>> + pr_err_ratelimited("Unable to get SGI status\n");
>> + goto out;
>> + }
>> + cpu_relax();
>> + udelay(1);
>> + } while(count);
>> +
>> +out:
>> + *val = !!(status & (1 << d->hwirq));
>> +
>> + return 0;
>> +}
>> +
>> static struct irq_chip its_sgi_irq_chip = {
>> .name = "GICv4.1-sgi",
>> .irq_mask = its_sgi_mask_irq,
>> .irq_unmask = its_sgi_unmask_irq,
>> .irq_set_affinity = its_sgi_set_affinity,
>> + .irq_set_irqchip_state = its_sgi_set_irqchip_state,
>> + .irq_get_irqchip_state = its_sgi_get_irqchip_state,
>> };
>> static int its_sgi_irq_domain_alloc(struct irq_domain *domain,
>> diff --git a/include/linux/irqchip/arm-gic-v3.h
>> b/include/linux/irqchip/arm-gic-v3.h
>> index a89578884263..64da945486ac 100644
>> --- a/include/linux/irqchip/arm-gic-v3.h
>> +++ b/include/linux/irqchip/arm-gic-v3.h
>> @@ -345,6 +345,15 @@
>> #define GICR_VPENDBASER_4_1_VGRP1EN (1ULL << 58)
>> #define GICR_VPENDBASER_4_1_VPEID GENMASK_ULL(15, 0)
>> +#define GICR_VSGIR 0x0080
>> +
>> +#define GICR_VSGIR_VPEID GENMASK(15, 0)
>> +
>> +#define GICR_VSGIPENDR 0x0088
>> +
>> +#define GICR_VSGIPENDR_BUSY (1U << 31)
>> +#define GICR_VSGIPENDR_PENDING GENMASK(15, 0)
>> +
>> /*
>> * ITS registers, offsets from ITS_base
>> */
>> @@ -368,6 +377,11 @@
>> #define GITS_TRANSLATER 0x10040
>> +#define GITS_SGIR 0x20020
>> +
>> +#define GITS_SGIR_VPEID GENMASK_ULL(47, 32)
>> +#define GITS_SGIR_VINTID GENMASK_ULL(7, 0)
>
> The spec says vINTID field is [3:0] of the GITS_SGIR.

Indeed, well spotted again!

Thanks,

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

\
 
 \ /
  Last update: 2020-02-18 10:28    [W:0.065 / U:0.596 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site