Messages in this thread | | | Date | Tue, 18 Feb 2020 09:27:44 +0000 | From | Marc Zyngier <> | Subject | Re: [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...
| |