Hi all,
This series is a mix of early GICv4.1 cleanups, fixes coming out of
discussions with Zenghui Yu, and a couple of stashed bug fixes that I
recently rediscovered (oops).
Hopefully nothing controvertial here, but please shout if you think
anything looks wrong. I've given it a good shake on my D05, and
everything was great (until the SSD containing the home directories
decided it had enough with life and everything ground to a halt).
As $SUBJECT says, I plan to take this into 5.5.
Thanks,
M.
Marc Zyngier (11):
irqchip/gic-v3-its: Free collection mapping on device teardown
irqchip/gic-v3-its: Factor out wait_for_syncr primitive
irqchip/gic-v3-its: Allow LPI invalidation via the DirectLPI interface
irqchip/gic-v3-its: Make is_v4 use a TYPER copy
irqchip/gic-v3-its: Kill its->ite_size and use TYPER copy instead
irqchip/gic-v3-its: Kill its->device_ids and use TYPER copy instead
irqchip/gic-v3-its: Add its_vlpi_map helpers
irqchip/gic-v3-its: Synchronise INV command targetting a VLPI using
VSYNC
irqchip/gic-v3-its: Synchronise INT/CLEAR commands targetting a VLPI
using VSYNC
irqchip/gic-v3-its: Lock VLPI map array before translating it
irqchip/gic-v3-its: Make vlpi_lock a spinlock
drivers/irqchip/irq-gic-v3-its.c | 288 ++++++++++++++++++++++-------
include/linux/irqchip/arm-gic-v3.h | 4 +-
2 files changed, 224 insertions(+), 68 deletions(-)
--
2.20.1
We have so far always invalidated VLPIs usinc an INV+SYNC
sequence, but that's pretty wrong for two reasons:
- SYNC only synchronises physical LPIs
- The collection ID that for the associated LPI doesn't match
the redistributor the vPE is associated with
Instead, send an INV+VSYNC for forwarded LPIs, ensuring that
the ITS can properly synchronise the invalidation of VLPIs.
Fixes: 015ec0386ab6 ("irqchip/gic-v3-its: Add VLPI configuration handling")
Reported-by: Zenghui Yu <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 36 +++++++++++++++++++++++++++++++-
1 file changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index c8c280f803a5..d4b9f12e2e53 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -700,6 +700,24 @@ static struct its_vpe *its_build_vmovp_cmd(struct its_node *its,
return valid_vpe(its, desc->its_vmovp_cmd.vpe);
}
+static struct its_vpe *its_build_vinv_cmd(struct its_node *its,
+ struct its_cmd_block *cmd,
+ struct its_cmd_desc *desc)
+{
+ struct its_vlpi_map *map;
+
+ map = dev_event_to_vlpi_map(desc->its_inv_cmd.dev,
+ desc->its_inv_cmd.event_id);
+
+ its_encode_cmd(cmd, GITS_CMD_INV);
+ its_encode_devid(cmd, desc->its_inv_cmd.dev->device_id);
+ its_encode_event_id(cmd, desc->its_inv_cmd.event_id);
+
+ its_fixup_cmd(cmd);
+
+ return valid_vpe(its, map->vpe);
+}
+
static u64 its_cmd_ptr_to_offset(struct its_node *its,
struct its_cmd_block *ptr)
{
@@ -1066,6 +1084,20 @@ static void its_send_vinvall(struct its_node *its, struct its_vpe *vpe)
its_send_single_vcommand(its, its_build_vinvall_cmd, &desc);
}
+static void its_send_vinv(struct its_device *dev, u32 event_id)
+{
+ struct its_cmd_desc desc;
+
+ /*
+ * There is no real VINV command. This is just a normal INV,
+ * with a VSYNC instead of a SYNC.
+ */
+ desc.its_inv_cmd.dev = dev;
+ desc.its_inv_cmd.event_id = event_id;
+
+ its_send_single_vcommand(dev->its, its_build_vinv_cmd, &desc);
+}
+
/*
* irqchip functions - assumes MSI, mostly.
*/
@@ -1140,8 +1172,10 @@ static void lpi_update_config(struct irq_data *d, u8 clr, u8 set)
lpi_write_config(d, clr, set);
if (gic_rdists->has_direct_lpi && !irqd_is_forwarded_to_vcpu(d))
direct_lpi_inv(d);
- else
+ else if (!irqd_is_forwarded_to_vcpu(d))
its_send_inv(its_dev, its_get_event_id(d));
+ else
+ its_send_vinv(its_dev, its_get_event_id(d));
}
static void its_vlpi_set_doorbell(struct irq_data *d, bool enable)
--
2.20.1
Waiting for a redistributor to have performed an operation is a
common thing to do, and the idiom is already spread around.
As we're going to make even more use of this, let's have a primitive
that does just that.
Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 15 +++++++++------
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 07d0bde60e16..d71741d302b4 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1090,6 +1090,12 @@ static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
dsb(ishst);
}
+static void wait_for_syncr(void __iomem *rdbase)
+{
+ while (gic_read_lpir(rdbase + GICR_SYNCR) & 1)
+ cpu_relax();
+}
+
static void lpi_update_config(struct irq_data *d, u8 clr, u8 set)
{
struct its_device *its_dev = irq_data_get_irq_chip_data(d);
@@ -2773,8 +2779,7 @@ static void its_vpe_db_proxy_move(struct its_vpe *vpe, int from, int to)
rdbase = per_cpu_ptr(gic_rdists->rdist, from)->rd_base;
gic_write_lpir(vpe->vpe_db_lpi, rdbase + GICR_CLRLPIR);
- while (gic_read_lpir(rdbase + GICR_SYNCR) & 1)
- cpu_relax();
+ wait_for_syncr(rdbase);
return;
}
@@ -2930,8 +2935,7 @@ static void its_vpe_send_inv(struct irq_data *d)
rdbase = per_cpu_ptr(gic_rdists->rdist, vpe->col_idx)->rd_base;
gic_write_lpir(vpe->vpe_db_lpi, rdbase + GICR_INVLPIR);
- while (gic_read_lpir(rdbase + GICR_SYNCR) & 1)
- cpu_relax();
+ wait_for_syncr(rdbase);
} else {
its_vpe_send_cmd(vpe, its_send_inv);
}
@@ -2973,8 +2977,7 @@ static int its_vpe_set_irqchip_state(struct irq_data *d,
gic_write_lpir(vpe->vpe_db_lpi, rdbase + GICR_SETLPIR);
} else {
gic_write_lpir(vpe->vpe_db_lpi, rdbase + GICR_CLRLPIR);
- while (gic_read_lpir(rdbase + GICR_SYNCR) & 1)
- cpu_relax();
+ wait_for_syncr(rdbase);
}
} else {
if (state)
--
2.20.1
Now that we have a copy of TYPER in the ITS structure, rely on this
to provide the same service as its->ite_size, which gets axed.
Errata workarounds are now updating the cached fields instead of
requiring a separate field in the ITS structure.
Signed-off-by: Marc Zyngier <[email protected]>
Reviewed-by: Zenghui Yu <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 8 ++++----
include/linux/irqchip/arm-gic-v3.h | 2 +-
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index a5d947b243f2..161831e2b7ac 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -6,6 +6,7 @@
#include <linux/acpi.h>
#include <linux/acpi_iort.h>
+#include <linux/bitfield.h>
#include <linux/bitmap.h>
#include <linux/cpu.h>
#include <linux/crash_dump.h>
@@ -108,7 +109,6 @@ struct its_node {
struct list_head its_device_list;
u64 flags;
unsigned long list_nr;
- u32 ite_size;
u32 device_ids;
int numa_node;
unsigned int msi_domain_flags;
@@ -2450,7 +2450,7 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
* sized as a power of two (and you need at least one bit...).
*/
nr_ites = max(2, nvecs);
- sz = nr_ites * its->ite_size;
+ 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);
if (alloc_lpis) {
@@ -3266,7 +3266,8 @@ static bool __maybe_unused its_enable_quirk_qdf2400_e0065(void *data)
struct its_node *its = data;
/* On QDF2400, the size of the ITE is 16Bytes */
- its->ite_size = 16;
+ its->typer &= ~GITS_TYPER_ITT_ENTRY_SIZE;
+ its->typer |= FIELD_PREP(GITS_TYPER_ITT_ENTRY_SIZE, 16 - 1);
return true;
}
@@ -3635,7 +3636,6 @@ static int __init its_probe_one(struct resource *res,
its->typer = typer;
its->base = its_base;
its->phys_base = res->start;
- its->ite_size = GITS_TYPER_ITT_ENTRY_SIZE(typer);
its->device_ids = GITS_TYPER_DEVBITS(typer);
if (is_v4(its)) {
if (!(typer & GITS_TYPER_VMOVP)) {
diff --git a/include/linux/irqchip/arm-gic-v3.h b/include/linux/irqchip/arm-gic-v3.h
index 5cc10cf7cb3e..4bce7a904075 100644
--- a/include/linux/irqchip/arm-gic-v3.h
+++ b/include/linux/irqchip/arm-gic-v3.h
@@ -334,7 +334,7 @@
#define GITS_TYPER_PLPIS (1UL << 0)
#define GITS_TYPER_VLPIS (1UL << 1)
#define GITS_TYPER_ITT_ENTRY_SIZE_SHIFT 4
-#define GITS_TYPER_ITT_ENTRY_SIZE(r) ((((r) >> GITS_TYPER_ITT_ENTRY_SIZE_SHIFT) & 0xf) + 1)
+#define GITS_TYPER_ITT_ENTRY_SIZE GENMASK_ULL(7, 4)
#define GITS_TYPER_IDBITS_SHIFT 8
#define GITS_TYPER_DEVBITS_SHIFT 13
#define GITS_TYPER_DEVBITS(r) ((((r) >> GITS_TYPER_DEVBITS_SHIFT) & 0x1f) + 1)
--
2.20.1
Somehow, we forgot to free the collection mapping when tearing down
a device, hence slowly leaking mapping arrays as devices get removed
from the system. That is, almost never.
Just to be safe, properly free the array on device teardown.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 787e8eec9a7f..07d0bde60e16 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -2471,6 +2471,7 @@ static void its_free_device(struct its_device *its_dev)
raw_spin_lock_irqsave(&its_dev->its->lock, flags);
list_del(&its_dev->entry);
raw_spin_unlock_irqrestore(&its_dev->its->lock, flags);
+ kfree(its_dev->event_map.col_map);
kfree(its_dev->itt);
kfree(its_dev);
}
--
2.20.1
The VLPI map is currently a mutex, and that's a bad idea as
this lock can be taken in non-preemptible contexts. Convert
it to a raw spinlock, and turn the memory allocation of the
VLPI map to be atomic.
Reported-by: Heyi Guo <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index 58ce231d5ade..93a39fcc2c96 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -132,7 +132,7 @@ struct event_lpi_map {
u16 *col_map;
irq_hw_number_t lpi_base;
int nr_lpis;
- struct mutex vlpi_lock;
+ raw_spinlock_t vlpi_lock;
struct its_vm *vm;
struct its_vlpi_map *vlpi_maps;
int nr_vlpis;
@@ -1433,13 +1433,13 @@ static int its_vlpi_map(struct irq_data *d, struct its_cmd_info *info)
if (!info->map)
return -EINVAL;
- mutex_lock(&its_dev->event_map.vlpi_lock);
+ raw_spin_lock(&its_dev->event_map.vlpi_lock);
if (!its_dev->event_map.vm) {
struct its_vlpi_map *maps;
maps = kcalloc(its_dev->event_map.nr_lpis, sizeof(*maps),
- GFP_KERNEL);
+ GFP_ATOMIC);
if (!maps) {
ret = -ENOMEM;
goto out;
@@ -1482,7 +1482,7 @@ static int its_vlpi_map(struct irq_data *d, struct its_cmd_info *info)
}
out:
- mutex_unlock(&its_dev->event_map.vlpi_lock);
+ raw_spin_unlock(&its_dev->event_map.vlpi_lock);
return ret;
}
@@ -1492,7 +1492,7 @@ static int its_vlpi_get(struct irq_data *d, struct its_cmd_info *info)
struct its_vlpi_map *map;
int ret = 0;
- mutex_lock(&its_dev->event_map.vlpi_lock);
+ raw_spin_lock(&its_dev->event_map.vlpi_lock);
map = get_vlpi_map(d);
@@ -1505,7 +1505,7 @@ static int its_vlpi_get(struct irq_data *d, struct its_cmd_info *info)
*info->map = *map;
out:
- mutex_unlock(&its_dev->event_map.vlpi_lock);
+ raw_spin_unlock(&its_dev->event_map.vlpi_lock);
return ret;
}
@@ -1515,7 +1515,7 @@ static int its_vlpi_unmap(struct irq_data *d)
u32 event = its_get_event_id(d);
int ret = 0;
- mutex_lock(&its_dev->event_map.vlpi_lock);
+ raw_spin_lock(&its_dev->event_map.vlpi_lock);
if (!its_dev->event_map.vm || !irqd_is_forwarded_to_vcpu(d)) {
ret = -EINVAL;
@@ -1545,7 +1545,7 @@ static int its_vlpi_unmap(struct irq_data *d)
}
out:
- mutex_unlock(&its_dev->event_map.vlpi_lock);
+ raw_spin_unlock(&its_dev->event_map.vlpi_lock);
return ret;
}
@@ -2605,7 +2605,7 @@ static struct its_device *its_create_device(struct its_node *its, u32 dev_id,
dev->event_map.col_map = col_map;
dev->event_map.lpi_base = lpi_base;
dev->event_map.nr_lpis = nr_lpis;
- mutex_init(&dev->event_map.vlpi_lock);
+ raw_spin_lock_init(&dev->event_map.vlpi_lock);
dev->device_id = dev_id;
INIT_LIST_HEAD(&dev->entry);
--
2.20.1
Obtaining the mapping information for a VLPI should always be
done with the vlpi_lock for this device held. Otherwise, we
expose ourselves to races against a concurrent unmap.
Signed-off-by: Marc Zyngier <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index dbb994f52e42..58ce231d5ade 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1489,12 +1489,14 @@ static int its_vlpi_map(struct irq_data *d, struct its_cmd_info *info)
static int its_vlpi_get(struct irq_data *d, struct its_cmd_info *info)
{
struct its_device *its_dev = irq_data_get_irq_chip_data(d);
- struct its_vlpi_map *map = get_vlpi_map(d);
+ struct its_vlpi_map *map;
int ret = 0;
mutex_lock(&its_dev->event_map.vlpi_lock);
- if (!its_dev->event_map.vm || !map->vm) {
+ map = get_vlpi_map(d);
+
+ if (!its_dev->event_map.vm || !map) {
ret = -EINVAL;
goto out;
}
--
2.20.1
Hi Marc,
On 2019/11/6 0:22, Marc Zyngier wrote:
> Somehow, we forgot to free the collection mapping when tearing down
> a device, hence slowly leaking mapping arrays as devices get removed
> from the system. That is, almost never.
>
> Just to be safe, properly free the array on device teardown.
>
> Signed-off-by: Marc Zyngier <[email protected]>
> ---
> drivers/irqchip/irq-gic-v3-its.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 787e8eec9a7f..07d0bde60e16 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -2471,6 +2471,7 @@ static void its_free_device(struct its_device *its_dev)
> raw_spin_lock_irqsave(&its_dev->its->lock, flags);
> list_del(&its_dev->entry);
> raw_spin_unlock_irqrestore(&its_dev->its->lock, flags);
> + kfree(its_dev->event_map.col_map);
I agreed that this is the appropriate place to free the collection
mapping (act as the counterpart of the allocation which happened in
its_create_device). But as pointed out by Heyi [1], this will
introduce a double free issue. We'd better also drop the kfree()
in its_irq_domain_free() in this patch?
(I find that it had been dropped in the last patch in your
irq/gic-5.5-wip branch, but maybe better here.)
[1] https://lkml.org/lkml/2019/7/15/329
Thanks,
Zenghui
> kfree(its_dev->itt);
> kfree(its_dev);
> }
>
Hi Zenghui,
On 2019-11-08 14:09, Zenghui Yu wrote:
> Hi Marc,
>
> On 2019/11/6 0:22, Marc Zyngier wrote:
>> Somehow, we forgot to free the collection mapping when tearing down
>> a device, hence slowly leaking mapping arrays as devices get removed
>> from the system. That is, almost never.
>> Just to be safe, properly free the array on device teardown.
>> Signed-off-by: Marc Zyngier <[email protected]>
>> ---
>> drivers/irqchip/irq-gic-v3-its.c | 1 +
>> 1 file changed, 1 insertion(+)
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index 787e8eec9a7f..07d0bde60e16 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -2471,6 +2471,7 @@ static void its_free_device(struct its_device
>> *its_dev)
>> raw_spin_lock_irqsave(&its_dev->its->lock, flags);
>> list_del(&its_dev->entry);
>> raw_spin_unlock_irqrestore(&its_dev->its->lock, flags);
>> + kfree(its_dev->event_map.col_map);
>
> I agreed that this is the appropriate place to free the collection
> mapping (act as the counterpart of the allocation which happened in
> its_create_device). But as pointed out by Heyi [1], this will
> introduce a double free issue. We'd better also drop the kfree()
> in its_irq_domain_free() in this patch?
>
> (I find that it had been dropped in the last patch in your
> irq/gic-5.5-wip branch, but maybe better here.)
Ah, that hunk is in a separate patch that I wasn't really
planning to send for this round. Let me fix the series (again)
and resend it...
Thanks for the heads up,
M.
--
Jazz is not dead. It just smells funny...