2021-12-08 01:53:43

by Zhangshaokun

[permalink] [raw]
Subject: [PATCH] irqchip/irq-gic-v3-its.c: Return its_invall_cmd.col when building INVALL

From: Wudi Wang <[email protected]>

INVALL CMD specifies that the ITS must ensure any caching associated with
the interrupt collection defined by ICID is consistent with the LPI
configuration tables held in memory for all Redistributors. SYNC is
required to ensure that INVALL is executed.

Currently, LPI configuration data may be inconsistent with that in the
memory within a short period of time after the INVALL command is executed.

Cc: Thomas Gleixner <[email protected]>
Cc: Marc Zyngier <[email protected]>
Signed-off-by: Wudi Wang <[email protected]>
Signed-off-by: Shaokun Zhang <[email protected]>
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index eb0882d15366..0cb584d9815b 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -742,7 +742,7 @@ static struct its_collection *its_build_invall_cmd(struct its_node *its,

its_fixup_cmd(cmd);

- return NULL;
+ return desc->its_invall_cmd.col;
}

static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,
--
2.33.0



2021-12-08 08:25:52

by Marc Zyngier

[permalink] [raw]
Subject: Re: [PATCH] irqchip/irq-gic-v3-its.c: Return its_invall_cmd.col when building INVALL

On 2021-12-08 01:54, Shaokun Zhang wrote:
> From: Wudi Wang <[email protected]>
>
> INVALL CMD specifies that the ITS must ensure any caching associated
> with
> the interrupt collection defined by ICID is consistent with the LPI
> configuration tables held in memory for all Redistributors. SYNC is
> required to ensure that INVALL is executed.

The patch title doesn't quite spell out the issue. It should say
something
like:

"Force synchronisation when issuing INVALL"

>
> Currently, LPI configuration data may be inconsistent with that in the
> memory within a short period of time after the INVALL command is
> executed.

I'm curious: have you seen any issue with this on actual HW? In my
experience, all implementations treat INVALL as a synchronous command,

Or was this solely done via inspection?

>
> Cc: Thomas Gleixner <[email protected]>
> Cc: Marc Zyngier <[email protected]>
> Signed-off-by: Wudi Wang <[email protected]>
> Signed-off-by: Shaokun Zhang <[email protected]>

This needs:

Fixes: cc2d3216f53 ("irqchip: GICv3: ITS command queue")

> ---
> drivers/irqchip/irq-gic-v3-its.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/irqchip/irq-gic-v3-its.c
> b/drivers/irqchip/irq-gic-v3-its.c
> index eb0882d15366..0cb584d9815b 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -742,7 +742,7 @@ static struct its_collection
> *its_build_invall_cmd(struct its_node *its,
>
> its_fixup_cmd(cmd);
>
> - return NULL;
> + return desc->its_invall_cmd.col;
> }
>
> static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,

I'll fix the above locally, no need to resend.

Thanks,

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

Subject: [irqchip: irq/irqchip-fixes] irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID: d094b4332232c88d07d9884a9c32fec259984351
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/d094b4332232c88d07d9884a9c32fec259984351
Author: Wudi Wang <[email protected]>
AuthorDate: Wed, 08 Dec 2021 09:54:29 +08:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Wed, 08 Dec 2021 08:34:09

irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

INVALL CMD specifies that the ITS must ensure any caching associated with
the interrupt collection defined by ICID is consistent with the LPI
configuration tables held in memory for all Redistributors. SYNC is
required to ensure that INVALL is executed.

Currently, LPI configuration data may be inconsistent with that in the
memory within a short period of time after the INVALL command is executed.

Signed-off-by: Wudi Wang <[email protected]>
Signed-off-by: Shaokun Zhang <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Fixes: cc2d3216f53 ("irqchip: GICv3: ITS command queue")
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index eb0882d..0cb584d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -742,7 +742,7 @@ static struct its_collection *its_build_invall_cmd(struct its_node *its,

its_fixup_cmd(cmd);

- return NULL;
+ return desc->its_invall_cmd.col;
}

static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,

Subject: [irqchip: irq/irqchip-fixes] irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

The following commit has been merged into the irq/irqchip-fixes branch of irqchip:

Commit-ID: b383a42ca523ce54bcbd63f7c8f3cf974abc9b9a
Gitweb: https://git.kernel.org/pub/scm/linux/kernel/git/maz/arm-platforms/b383a42ca523ce54bcbd63f7c8f3cf974abc9b9a
Author: Wudi Wang <[email protected]>
AuthorDate: Wed, 08 Dec 2021 09:54:29 +08:00
Committer: Marc Zyngier <[email protected]>
CommitterDate: Wed, 08 Dec 2021 11:13:18

irqchip/irq-gic-v3-its.c: Force synchronisation when issuing INVALL

INVALL CMD specifies that the ITS must ensure any caching associated with
the interrupt collection defined by ICID is consistent with the LPI
configuration tables held in memory for all Redistributors. SYNC is
required to ensure that INVALL is executed.

Currently, LPI configuration data may be inconsistent with that in the
memory within a short period of time after the INVALL command is executed.

Signed-off-by: Wudi Wang <[email protected]>
Signed-off-by: Shaokun Zhang <[email protected]>
Signed-off-by: Marc Zyngier <[email protected]>
Fixes: cc2d3216f53c ("irqchip: GICv3: ITS command queue")
Link: https://lore.kernel.org/r/[email protected]
---
drivers/irqchip/irq-gic-v3-its.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
index eb0882d..0cb584d 100644
--- a/drivers/irqchip/irq-gic-v3-its.c
+++ b/drivers/irqchip/irq-gic-v3-its.c
@@ -742,7 +742,7 @@ static struct its_collection *its_build_invall_cmd(struct its_node *its,

its_fixup_cmd(cmd);

- return NULL;
+ return desc->its_invall_cmd.col;
}

static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,

2021-12-09 01:17:30

by Zhangshaokun

[permalink] [raw]
Subject: Re: [PATCH] irqchip/irq-gic-v3-its.c: Return its_invall_cmd.col when building INVALL

Hi Marc,

On 2021/12/8 16:25, Marc Zyngier wrote:
> On 2021-12-08 01:54, Shaokun Zhang wrote:
>> From: Wudi Wang <[email protected]>
>>
>> INVALL CMD specifies that the ITS must ensure any caching associated with
>> the interrupt collection defined by ICID is consistent with the LPI
>> configuration tables held in memory for all Redistributors. SYNC is
>> required to ensure that INVALL is executed.
>
> The patch title doesn't quite spell out the issue. It should say something
> like:
>
> "Force synchronisation when issuing INVALL">

Make sense.

>>
>> Currently, LPI configuration data may be inconsistent with that in the
>> memory within a short period of time after the INVALL command is executed.
>
> I'm curious: have you seen any issue with this on actual HW? In my
> experience, all implementations treat INVALL as a synchronous command,
>
> Or was this solely done via inspection?
>

It is noticed by checking the implementation of INVALL API function, not
by on actual HW.

>>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: Marc Zyngier <[email protected]>
>> Signed-off-by: Wudi Wang <[email protected]>
>> Signed-off-by: Shaokun Zhang <[email protected]>
>
> This needs:
>
> Fixes: cc2d3216f53 ("irqchip: GICv3: ITS command queue")
>

Oops, indeed, apologies that forget to add this tag.

>> ---
>>  drivers/irqchip/irq-gic-v3-its.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
>> index eb0882d15366..0cb584d9815b 100644
>> --- a/drivers/irqchip/irq-gic-v3-its.c
>> +++ b/drivers/irqchip/irq-gic-v3-its.c
>> @@ -742,7 +742,7 @@ static struct its_collection
>> *its_build_invall_cmd(struct its_node *its,
>>
>>      its_fixup_cmd(cmd);
>>
>> -    return NULL;
>> +    return desc->its_invall_cmd.col;
>>  }
>>
>>  static struct its_vpe *its_build_vinvall_cmd(struct its_node *its,
>
> I'll fix the above locally, no need to resend.
>

Thanks Marc's help.

> Thanks,
>
>         M.