Re: [PATCH] irqchip/gicv3-its: Add workaround for hip09 ITS erratum 162100801

[Marc Zyngier <maz@kernel.org>]

On Mon, 04 Nov 2024 12:11:43 +0000,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> When enabled GICv4.1 in hip09, VMAPP will fail to clear some caches

nit: enabling.

> during unmapping operation, which will cause some vSGIs interrupts lost.
> 
> To fix the issue, it needs to send vinvall command after vmovp.

nit: commands need to be in capital letters (VINVALL, VMOVP). drop
'interrupts'.

>
> Signed-off-by: Nianyao Tang <tangnianyao@huawei.com>
> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> ---
>  Documentation/arch/arm64/silicon-errata.rst |  2 ++
>  arch/arm64/Kconfig                          | 10 ++++++
>  drivers/irqchip/irq-gic-v3-its.c            | 36 ++++++++++++++++-----
>  3 files changed, 40 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
> index 65bfab1b1861..77db10e944f0 100644
> --- a/Documentation/arch/arm64/silicon-errata.rst
> +++ b/Documentation/arch/arm64/silicon-errata.rst
> @@ -258,6 +258,8 @@ stable kernels.
>  | Hisilicon      | Hip{08,09,10,10C| #162001900      | N/A                         |
>  |                | ,11} SMMU PMCG  |                 |                             |
>  +----------------+-----------------+-----------------+-----------------------------+
> +| Hisilicon      | Hip09           | #162100801      | HISILICON_ERRATUM_162100801 |
> ++----------------+-----------------+-----------------+-----------------------------+
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Qualcomm Tech. | Kryo/Falkor v1  | E1003           | QCOM_FALKOR_ERRATUM_1003    |
>  +----------------+-----------------+-----------------+-----------------------------+
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index fd9df6dcc593..27082e075d1a 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1232,6 +1232,16 @@ config HISILICON_ERRATUM_161600802
>  
>  	  If unsure, say Y.
>  
> +config HISILICON_ERRATUM_162100801
> +	bool "Hip09 162100801 erratum support"
> +	default y
> +	help
> +	  When enabled GICv4.1 in hip09, VMAPP will fail to clear some caches
> +	  during unmapping operation, which will cause some vSGIs interrupts
> +	  lost. So fix it by sending vinvall commands after vmovp.

Same remarks as above.

Now, the workaround seems odd. I understand from the description that
VMAPP with V=0 results in leftover cached entries influencing the
behaviour of a newly created VPE.

But if it is a VINVALL on each CPU that cures it, why don't we do that
upfront, instead of on every single VMOVP?

Or is the problem that VMAPP(V=0) can result in *existing* VPEs to
observe an unrelated (or corrupted) state?

> +
> +	  If unsure, say Y.
> +
>  config QCOM_FALKOR_ERRATUM_1003
>  	bool "Falkor E1003: Incorrect translation due to ASID change"
>  	default y
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 52f625e07658..69b09072d24d 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -44,6 +44,7 @@
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>  #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
>  #define ITS_FLAGS_FORCE_NON_SHAREABLE		(1ULL << 3)
> +#define ITS_FLAGS_WORKAROUND_HISILICON_162100801	(1ULL << 4)
>  
>  #define RD_LOCAL_LPI_ENABLED                    BIT(0)
>  #define RD_LOCAL_PENDTABLE_PREALLOCATED         BIT(1)
> @@ -1314,6 +1315,14 @@ static void its_send_vmapp(struct its_node *its,
>  	its_send_single_vcommand(its, its_build_vmapp_cmd, &desc);
>  }
>  
> +static void its_send_vinvall(struct its_node *its, struct its_vpe *vpe)
> +{
> +	struct its_cmd_desc desc;
> +
> +	desc.its_vinvall_cmd.vpe = vpe;
> +	its_send_single_vcommand(its, its_build_vinvall_cmd, &desc);
> +}
> +
>  static void its_send_vmovp(struct its_vpe *vpe)
>  {
>  	struct its_cmd_desc desc = {};
> @@ -1351,17 +1360,12 @@ static void its_send_vmovp(struct its_vpe *vpe)
>  
>  		desc.its_vmovp_cmd.col = &its->collections[col_id];
>  		its_send_single_vcommand(its, its_build_vmovp_cmd, &desc);
> +		if (is_v4_1(its) && (its->flags &
> +			    ITS_FLAGS_WORKAROUND_HISILICON_162100801))
> +			its_send_vinvall(its, vpe);
>  	}
>  }

Please don't bury workarounds inside the command primitives (and VMOVP
is complicated enough). There is a single place where we send VMOVP,
and I'd rather you place the workaround there.

But more importantly, this code isn't supposed to get executed with a
GICv4.1 implementation, as the driver does not expect it to use an
ITSList.

So what is your system implementing? Do you have such thing as GICv4.1
with ITSList? If so, I'm pretty sure the driver is broken on with such
mix...

Thanks,

	M.

-- 
Without deviation from the norm, progress is not possible.