| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 18 Feb 2020 09:27:44 +0000
From: Marc Zyngier <maz@...nel.org>
To: Zenghui Yu <yuzenghui@...wei.com>
Cc: linux-arm-kernel@...ts.infradead.org, kvmarm@...ts.cs.columbia.edu,
kvm@...r.kernel.org, linux-kernel@...r.kernel.org,
Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
Jason Cooper <jason@...edaemon.net>,
Robert Richter <rrichter@...vell.com>,
Thomas Gleixner <tglx@...utronix.de>,
Eric Auger <eric.auger@...hat.com>,
James Morse <james.morse@....com>,
Julien Thierry <julien.thierry.kdev@...il.com>,
Suzuki K Poulose <suzuki.poulose@....com>
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@...nel.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...
Powered by blists - more mailing lists