[PATCH] irqchip/gic-v3-its: Don't confuse get_vlpi_map() by writing DB config

[PATCH] irqchip/gic-v3-its: Don't confuse get_vlpi_map() by writing DB config

[Zenghui Yu]

When we're writing config for the doorbell interrupt, get_vlpi_map() will
get confused by doorbell's d->parent_data hack and find the wrong its_dev
as chip data and the wrong event.

Fix this issue by making sure no doorbells will be involved before invoking
get_vlpi_map(), which restore some of the logic in lpi_write_config().

Fixes: c1d4d5cd203c ("irqchip/gic-v3-its: Add its_vlpi_map helpers")
Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
---

This is based on mainline and can't be directly applied to the current
irqchip-next.

 drivers/irqchip/irq-gic-v3-its.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e05673bcd52b..cc8a4fcbd6d6 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1181,12 +1181,13 @@ static struct its_vlpi_map *get_vlpi_map(struct irq_data *d)
 
 static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
 {
-	struct its_vlpi_map *map = get_vlpi_map(d);
 	irq_hw_number_t hwirq;
 	void *va;
 	u8 *cfg;
 
-	if (map) {
+	if (irqd_is_forwarded_to_vcpu(d)) {
+		struct its_vlpi_map *map = get_vlpi_map(d);
+
 		va = page_address(map->vm->vprop_page);
 		hwirq = map->vintid;
 
-- 
2.19.1

Re: [PATCH] irqchip/gic-v3-its: Don't confuse get_vlpi_map() by writing DB config

[Marc Zyngier]

Hi Zenghui,

Thanks for this.

On 2020-01-22 08:56, Zenghui Yu wrote:
> When we're writing config for the doorbell interrupt, get_vlpi_map() 
> will
> get confused by doorbell's d->parent_data hack and find the wrong 
> its_dev
> as chip data and the wrong event.
> 
> Fix this issue by making sure no doorbells will be involved before 
> invoking
> get_vlpi_map(), which restore some of the logic in lpi_write_config().
> 
> Fixes: c1d4d5cd203c ("irqchip/gic-v3-its: Add its_vlpi_map helpers")
> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
> ---
> 
> This is based on mainline and can't be directly applied to the current
> irqchip-next.
> 
>  drivers/irqchip/irq-gic-v3-its.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index e05673bcd52b..cc8a4fcbd6d6 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1181,12 +1181,13 @@ static struct its_vlpi_map
> *get_vlpi_map(struct irq_data *d)
> 
>  static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
>  {
> -	struct its_vlpi_map *map = get_vlpi_map(d);
>  	irq_hw_number_t hwirq;
>  	void *va;
>  	u8 *cfg;
> 
> -	if (map) {
> +	if (irqd_is_forwarded_to_vcpu(d)) {
> +		struct its_vlpi_map *map = get_vlpi_map(d);
> +
>  		va = page_address(map->vm->vprop_page);
>  		hwirq = map->vintid;

Shouldn't we fix get_vlpi_map() instead? Something like (untested):

diff --git a/drivers/irqchip/irq-gic-v3-its.c 
b/drivers/irqchip/irq-gic-v3-its.c
index e05673bcd52b..b704214390c0 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1170,13 +1170,14 @@ static void its_send_vclear(struct its_device 
*dev, u32 event_id)
   */
  static struct its_vlpi_map *get_vlpi_map(struct irq_data *d)
  {
-	struct its_device *its_dev = irq_data_get_irq_chip_data(d);
-	u32 event = its_get_event_id(d);
+	if (irqd_is_forwarded_to_vcpu(d)) {
+		struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+		u32 event = its_get_event_id(d);

-	if (!irqd_is_forwarded_to_vcpu(d))
-		return NULL;
+		return dev_event_to_vlpi_map(its_dev, event);
+	}

-	return dev_event_to_vlpi_map(its_dev, event);
+	return NULL;
  }

  static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)


Or am I missing the actual problem?

Overall, I'm starting to hate that ->parent hack as it's been the source
of a number of bugs.

The main issue is that the VPE hierarchy is missing one level (it has
no ITS domain, and goes directly from the VPE domain to the low-level
GIC domain). It means we end-up special-casing things, and that's never
good...

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

Re: [PATCH] irqchip/gic-v3-its: Don't confuse get_vlpi_map() by writing DB config

[Zenghui Yu]

Hi Marc,

On 2020/1/22 18:44, Marc Zyngier wrote:
> Hi Zenghui,
> 
> Thanks for this.
> 
> On 2020-01-22 08:56, Zenghui Yu wrote:
>> When we're writing config for the doorbell interrupt, get_vlpi_map() will
>> get confused by doorbell's d->parent_data hack and find the wrong its_dev
>> as chip data and the wrong event.
>>
>> Fix this issue by making sure no doorbells will be involved before 
>> invoking
>> get_vlpi_map(), which restore some of the logic in lpi_write_config().
>>
>> Fixes: c1d4d5cd203c ("irqchip/gic-v3-its: Add its_vlpi_map helpers")
>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>> ---
>>
>> This is based on mainline and can't be directly applied to the current
>> irqchip-next.
>>
>>  drivers/irqchip/irq-gic-v3-its.c | 5 +++--
>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index e05673bcd52b..cc8a4fcbd6d6 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1181,12 +1181,13 @@ static struct its_vlpi_map
>> *get_vlpi_map(struct irq_data *d)
>>
>>  static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
>>  {
>> -    struct its_vlpi_map *map = get_vlpi_map(d);
>>      irq_hw_number_t hwirq;
>>      void *va;
>>      u8 *cfg;
>>
>> -    if (map) {
>> +    if (irqd_is_forwarded_to_vcpu(d)) {
>> +        struct its_vlpi_map *map = get_vlpi_map(d);
>> +
>>          va = page_address(map->vm->vprop_page);
>>          hwirq = map->vintid;
> 
> Shouldn't we fix get_vlpi_map() instead? Something like (untested):
> 
> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
> b/drivers/irqchip/irq-gic-v3-its.c
> index e05673bcd52b..b704214390c0 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -1170,13 +1170,14 @@ static void its_send_vclear(struct its_device 
> *dev, u32 event_id)
>    */
>   static struct its_vlpi_map *get_vlpi_map(struct irq_data *d)
>   {
> -    struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> -    u32 event = its_get_event_id(d);
> +    if (irqd_is_forwarded_to_vcpu(d)) {
> +        struct its_device *its_dev = irq_data_get_irq_chip_data(d);
> +        u32 event = its_get_event_id(d);
> 
> -    if (!irqd_is_forwarded_to_vcpu(d))
> -        return NULL;
> +        return dev_event_to_vlpi_map(its_dev, event);
> +    }
> 
> -    return dev_event_to_vlpi_map(its_dev, event);
> +    return NULL;
>   }
> 
>   static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
> 
> 
> Or am I missing the actual problem?

No. I also thought about fixing the issue by this way and I'm OK with
both approaches.

> 
> Overall, I'm starting to hate that ->parent hack as it's been the source
> of a number of bugs.

(thankfully it's rarely used and we've so far handled them carefully,
except the lpi_write_config issue in this patch...)

> 
> The main issue is that the VPE hierarchy is missing one level (it has
> no ITS domain, and goes directly from the VPE domain to the low-level
> GIC domain). It means we end-up special-casing things, and that's never
> good...

Yeah, this may comes from the fact that the per-vPE doorbell is not
served by ITS and can be asserted by redistributor directly. And the
special doorbell is programmed together with those normal LPI
(translated from MSI by ITS).


Thanks,
Zenghui

Re: [PATCH] irqchip/gic-v3-its: Don't confuse get_vlpi_map() by writing DB config

[Marc Zyngier]

On 2020-01-22 11:29, Zenghui Yu wrote:
> Hi Marc,
> 
> On 2020/1/22 18:44, Marc Zyngier wrote:
>> Hi Zenghui,
>> 
>> Thanks for this.
>> 
>> On 2020-01-22 08:56, Zenghui Yu wrote:
>>> When we're writing config for the doorbell interrupt, get_vlpi_map() 
>>> will
>>> get confused by doorbell's d->parent_data hack and find the wrong 
>>> its_dev
>>> as chip data and the wrong event.
>>> 
>>> Fix this issue by making sure no doorbells will be involved before 
>>> invoking
>>> get_vlpi_map(), which restore some of the logic in 
>>> lpi_write_config().
>>> 
>>> Fixes: c1d4d5cd203c ("irqchip/gic-v3-its: Add its_vlpi_map helpers")
>>> Signed-off-by: Zenghui Yu <yuzenghui@huawei.com>
>>> ---
>>> 
>>> This is based on mainline and can't be directly applied to the 
>>> current
>>> irqchip-next.
>>> 
>>>  drivers/irqchip/irq-gic-v3-its.c | 5 +++--
>>>  1 file changed, 3 insertions(+), 2 deletions(-)
>>> 
>>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>>> b/drivers/irqchip/irq-gic-v3-its.c
>>> index e05673bcd52b..cc8a4fcbd6d6 100644
>>> --- a/drivers/irqchip/irq-gic-v3-its.c
>>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>>> @@ -1181,12 +1181,13 @@ static struct its_vlpi_map
>>> *get_vlpi_map(struct irq_data *d)
>>> 
>>>  static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
>>>  {
>>> -    struct its_vlpi_map *map = get_vlpi_map(d);
>>>      irq_hw_number_t hwirq;
>>>      void *va;
>>>      u8 *cfg;
>>> 
>>> -    if (map) {
>>> +    if (irqd_is_forwarded_to_vcpu(d)) {
>>> +        struct its_vlpi_map *map = get_vlpi_map(d);
>>> +
>>>          va = page_address(map->vm->vprop_page);
>>>          hwirq = map->vintid;
>> 
>> Shouldn't we fix get_vlpi_map() instead? Something like (untested):
>> 
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c 
>> b/drivers/irqchip/irq-gic-v3-its.c
>> index e05673bcd52b..b704214390c0 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -1170,13 +1170,14 @@ static void its_send_vclear(struct its_device 
>> *dev, u32 event_id)
>>    */
>>   static struct its_vlpi_map *get_vlpi_map(struct irq_data *d)
>>   {
>> -    struct its_device *its_dev = irq_data_get_irq_chip_data(d);
>> -    u32 event = its_get_event_id(d);
>> +    if (irqd_is_forwarded_to_vcpu(d)) {
>> +        struct its_device *its_dev = irq_data_get_irq_chip_data(d);
>> +        u32 event = its_get_event_id(d);
>> 
>> -    if (!irqd_is_forwarded_to_vcpu(d))
>> -        return NULL;
>> +        return dev_event_to_vlpi_map(its_dev, event);
>> +    }
>> 
>> -    return dev_event_to_vlpi_map(its_dev, event);
>> +    return NULL;
>>   }
>> 
>>   static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)
>> 
>> 
>> Or am I missing the actual problem?
> 
> No. I also thought about fixing the issue by this way and I'm OK with
> both approaches.

OK, thanks. I've added this to irqchip-next[1], and rebased the v4.1
series on top of it. That way, the fix will trickle down to stable
without conflicts.

I've also given it a go on D05 with GICv4 enabled, and nothing caught 
fire.

         M.

[1] 
https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms.git/commit/?h=irq/irqchip-next&id=093bf439fee0d40ade7e309c1288b409cdc3b38f
-- 
Jazz is not dead. It just smells funny...

[tip: irq/core] irqchip/gic-v3-its: Fix get_vlpi_map() breakage with doorbells

[tip-bot2 for Marc Zyngier]

The following commit has been merged into the irq/core branch of tip:

Commit-ID:     093bf439fee0d40ade7e309c1288b409cdc3b38f
Gitweb:        https://git.kernel.org/tip/093bf439fee0d40ade7e309c1288b409cdc3b38f
Author:        Marc Zyngier <maz@kernel.org>
AuthorDate:    Wed, 22 Jan 2020 13:53:44 
Committer:     Marc Zyngier <maz@kernel.org>
CommitterDate: Wed, 22 Jan 2020 14:21:07 

irqchip/gic-v3-its: Fix get_vlpi_map() breakage with doorbells

When updating an LPI configuration, get_vlpi_map() may be passed a
irq_data structure relative to an ITS domain (the normal case) or one
that is relative to the core GICv3 domain in the case of a GICv4
doorbell.

In the latter case, special care must be take not to dereference
the irq_chip data as an its_dev structure, as that isn't what is
stored there. Instead, check *first* whether the IRQ is forwarded
to a vcpu, and only then try to obtain the vlpi mapping.

Fixes: c1d4d5cd203c ("irqchip/gic-v3-its: Add its_vlpi_map helpers")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Reported-by: Zenghui Yu <yuzenghui@huawei.com>
Link: https://lore.kernel.org/r/20200122085609.658-1-yuzenghui@huawei.com
---
 drivers/irqchip/irq-gic-v3-its.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index e05673b..b704214 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -1170,13 +1170,14 @@ static void its_send_vclear(struct its_device *dev, u32 event_id)
  */
 static struct its_vlpi_map *get_vlpi_map(struct irq_data *d)
 {
-	struct its_device *its_dev = irq_data_get_irq_chip_data(d);
-	u32 event = its_get_event_id(d);
+	if (irqd_is_forwarded_to_vcpu(d)) {
+		struct its_device *its_dev = irq_data_get_irq_chip_data(d);
+		u32 event = its_get_event_id(d);
 
-	if (!irqd_is_forwarded_to_vcpu(d))
-		return NULL;
+		return dev_event_to_vlpi_map(its_dev, event);
+	}
 
-	return dev_event_to_vlpi_map(its_dev, event);
+	return NULL;
 }
 
 static void lpi_write_config(struct irq_data *d, u8 clr, u8 set)