Re: [PATCH] irqchip/gicv3-its: Add workaround for HIP09/HIP10/HIP10C erratum 162100803/162200807/162400807

[Marc Zyngier <maz@kernel.org>]

On Tue, 09 Sep 2025 12:06:15 +0100,
Zhou Wang <wangzhou1@hisilicon.com> wrote:
> 
> HIP09/HIP10/HIP10C ITS have a problem, an ITS RAS will be reported in
> some cases when GICv4.1 is enable.

Do you mean a RAS *error*? Why can't the firmware ignore this event instead?

> A workaround is that set ITT size to max value(0xf) when doing MAPD with V = 1,
> and avoid to send MAPD with V = 0 to hardware. Just clear V field in ITS device
> table and clear ITS cache to implement MAPD with V = 0 instead.
> 
> Signed-off-by: Zhou Wang <wangzhou1@hisilicon.com>
> Reviewed-by: Nianyao Tang <tangnianyao@huawei.com>
> Reviewed-by: Kunkun Jiang <jiangkunkun@huawei.com>
> ---
>  Documentation/arch/arm64/silicon-errata.rst |   6 ++
>  arch/arm64/Kconfig                          |  13 +++
>  drivers/irqchip/irq-gic-v3-its.c            | 114 +++++++++++++++++---
>  3 files changed, 119 insertions(+), 14 deletions(-)
> 
> diff --git a/Documentation/arch/arm64/silicon-errata.rst b/Documentation/arch/arm64/silicon-errata.rst
> index b18ef4064bc0..dfafc608dc57 100644
> --- a/Documentation/arch/arm64/silicon-errata.rst
> +++ b/Documentation/arch/arm64/silicon-errata.rst
> @@ -264,6 +264,12 @@ stable kernels.
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Hisilicon      | Hip09           | #162100801      | HISILICON_ERRATUM_162100801 |
>  +----------------+-----------------+-----------------+-----------------------------+
> +| Hisilicon      | Hip09           | #162100803      | HISILICON_ERRATUM_162100803 |
> ++----------------+-----------------+-----------------+-----------------------------+
> +| Hisilicon      | Hip10           | #162200807      | HISILICON_ERRATUM_162100803 |
> ++----------------+-----------------+-----------------+-----------------------------+
> +| Hisilicon      | Hip10c          | #162400807      | HISILICON_ERRATUM_162100803 |
> ++----------------+-----------------+-----------------+-----------------------------+
>  +----------------+-----------------+-----------------+-----------------------------+
>  | Qualcomm Tech. | Kryo/Falkor v1  | E1003           | QCOM_FALKOR_ERRATUM_1003    |
>  +----------------+-----------------+-----------------+-----------------------------+
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index e9bbfacc35a6..803df402c9af 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1270,6 +1270,19 @@ config HISILICON_ERRATUM_162100801
>  
>  	  If unsure, say Y.
>  
> +config HISILICON_ERRATUM_162100803
> +	bool "Hip09/10/10c 162100803/162200807/162400807 erratum support"
> +	default y
> +	help
> +	  There is a hardware conflict between vSGI and vLPI, fix it by
> +	  configure a max ITS ITT size 0xf when doing MAPD with V = 1, clear V
> +	  field in ITS device table and clear ITS cache to implement MAPD with
> +	  V = 0 instead. Hip09/10/10c have this same problem, just use
> +	  HISILICON_ERRATUM_162100803 as the compile macro and
> +	  ITS_FLAGS_WORKAROUND_HISILICON_162100803 as ITS flag for convenience.
> +

I don't think any of the details belong to Kconfig. You don't even
explain *what* the problem is (hardware conflict doesn't mean
much). It is also completely unclear what MAPD has to do with vSGI and
vLPI.

> +	  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 467cb78435a9..647bc70cc2f7 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -49,6 +49,7 @@
>  #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 ITS_FLAGS_WORKAROUND_HISILICON_162100803	(1ULL << 5)
>  
>  #define RD_LOCAL_LPI_ENABLED                    BIT(0)
>  #define RD_LOCAL_PENDTABLE_PREALLOCATED         BIT(1)
> @@ -716,7 +717,12 @@ static struct its_collection *its_build_mapd_cmd(struct its_node *its,
>  
>  	its_encode_cmd(cmd, GITS_CMD_MAPD);
>  	its_encode_devid(cmd, desc->its_mapd_cmd.dev->device_id);
> -	its_encode_size(cmd, size - 1);
> +
> +	if (its->flags & ITS_FLAGS_WORKAROUND_HISILICON_162100803)
> +		its_encode_size(cmd, 0xf);

You are telling the ITS that it is allowed to go and access
unspecified data (16 bit worth of translations). That's not
acceptable. If you *have* to do that, then override the size in the
driver code to actually allocate the corresponding memory.

> +	else
> +		its_encode_size(cmd, size - 1);
> +
>  	its_encode_itt(cmd, itt_addr);
>  	its_encode_valid(cmd, desc->its_mapd_cmd.valid);
>  
> @@ -725,6 +731,61 @@ static struct its_collection *its_build_mapd_cmd(struct its_node *its,
>  	return NULL;
>  }
>  
> +static struct its_baser *its_get_baser(struct its_node *its, u32 type)
> +{
> +	int i;
> +
> +	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> +		if (GITS_BASER_TYPE(its->tables[i].val) == type)
> +			return &its->tables[i];
> +	}
> +
> +	return NULL;
> +}
> +
> +static struct its_collection
> +*its_build_mapd_cmd_hisi_quirk(struct its_node *its, struct its_cmd_block *cmd,
> +			       struct its_cmd_desc *desc)
> +{
> +	struct its_baser *baser = its_get_baser(its, GITS_BASER_TYPE_DEVICE);
> +	u32 devid = desc->its_mapd_cmd.dev->device_id;
> +	void *base = baser->base;
> +	u32 *dt_entry;
> +	void __iomem *its_func_en = its->sgir_base + 0x80;
> +	u32 tmp, tmp1, mask = 1 << 19;
> +	int i = 100;
> +
> +	/*
> +	 * Modify v to 0 in the dt entry of devid, dt entry format as below:
> +	 * word0: [0-31] ITT address; word1: [0-7] ITT address, [8-12]: ITT
> +	 * size, [13]: V. Only support flat device table.

Do you mean that the HW only supports flat tables? Or that you expect
the kernel to only produce flat device tables?

> +	 */
> +	dt_entry = (u32 *)(base + devid * 8 + 4);
> +	*dt_entry &= ~(1 << 13);
> +
> +	dsb(ishst);

Is this always cacheable memory? Does the ITS always snoop the CPU
caches? What protects against a concurrent access?

> +
> +	/*
> +	 * Cache invalidate by a private register GITS_FUNC_EN, whose offset
> +	 * is 0x20080 of ITS base address. GICv4.1 already maps sgir_base
> +	 * (offset is 0x20000), so address of GITS_FUNC_EN can be got by
> +	 * sgir_base + 0x80. Bit 16 is used to clear DT cache, the flip of
> +	 * bit 19 indicates that DT cache has been cleared.
> +	 */
> +	while (--i) {
> +		tmp = readl_relaxed(its_func_en) & mask;
> +		writel_relaxed(tmp | (1 << 16), its_func_en);
> +		tmp1 = readl_relaxed(its_func_en) & mask;
> +		if (tmp != tmp1)
> +			break;
> +	}

Please define these bits so that they have names instead of using raw
values. Why do you need to write bit 16 every time? Surely once you
have written it *once*, the HW remember what it has been told to do?

And frankly, using readl_relaxed_poll_timeout_atomic should be the
right thing to do.

> +
> +	if (i == 0)
> +		WARN_ON(1);
> +
> +	return NULL;
> +}
> +
>  static struct its_collection *its_build_mapc_cmd(struct its_node *its,
>  						 struct its_cmd_block *cmd,
>  						 struct its_cmd_desc *desc)
> @@ -1306,11 +1367,18 @@ static void its_send_inv(struct its_device *dev, u32 event_id)
>  static void its_send_mapd(struct its_device *dev, int valid)
>  {
>  	struct its_cmd_desc desc;
> +	its_cmd_builder_t fn;
> +
> +	if (dev->its->flags & ITS_FLAGS_WORKAROUND_HISILICON_162100803 &&
> +	    valid == 0)
> +		fn = its_build_mapd_cmd_hisi_quirk;

Make the new helper handle both values of the valid bit.

> +	else
> +		fn = its_build_mapd_cmd;
>  
>  	desc.its_mapd_cmd.dev = dev;
>  	desc.its_mapd_cmd.valid = !!valid;
>  
> -	its_send_single_command(dev->its, its_build_mapd_cmd, &desc);
> +	its_send_single_command(dev->its, fn, &desc);
>  }
>  
>  static void its_send_mapc(struct its_node *its, struct its_collection *col,
> @@ -3354,18 +3422,6 @@ static struct its_device *its_find_device(struct its_node *its, u32 dev_id)
>  	return its_dev;
>  }
>  
> -static struct its_baser *its_get_baser(struct its_node *its, u32 type)
> -{
> -	int i;
> -
> -	for (i = 0; i < GITS_BASER_NR_REGS; i++) {
> -		if (GITS_BASER_TYPE(its->tables[i].val) == type)
> -			return &its->tables[i];
> -	}
> -
> -	return NULL;
> -}
> -
>  static bool its_alloc_table_entry(struct its_node *its,
>  				  struct its_baser *baser, u32 id)
>  {
> @@ -4902,6 +4958,16 @@ static bool __maybe_unused its_enable_rk3568002(void *data)
>  	return true;
>  }
>  
> +static bool __maybe_unused its_enable_quirk_162100803(void *data)
> +{
> +	struct its_node *its = data;
> +
> +	if (is_v4_1(its))
> +		its->flags |= ITS_FLAGS_WORKAROUND_HISILICON_162100803;
> +
> +	return true;
> +}
> +
>  static const struct gic_quirk its_quirks[] = {
>  #ifdef CONFIG_CAVIUM_ERRATUM_22375
>  	{
> @@ -4956,6 +5022,26 @@ static const struct gic_quirk its_quirks[] = {
>  		.init	= its_enable_quirk_hip09_162100801,
>  	},
>  #endif
> +#ifdef CONFIG_HISILICON_ERRATUM_162100803
> +	{
> +		.desc = "ITS: Hip09 erratum 162100803",
> +		.iidr = 0x00051736,
> +		.mask = 0xffffffff,
> +		.init = its_enable_quirk_162100803,
> +	},
> +	{
> +		.desc = "ITS: Hip10 erratum 162200807",
> +		.iidr = 0x01051736,
> +		.mask = 0xffffffff,
> +		.init = its_enable_quirk_162100803,
> +	},
> +	{
> +		.desc = "ITS: Hip10c erratum 162400807",
> +		.iidr = 0x00061736,
> +		.mask = 0xffffffff,
> +		.init = its_enable_quirk_162100803,

Can you try to merge these three entries and adjust the mask
accordingly? mask=0xeffcffff should do the trick, assuming that you
don't have any other hardware overlapping this.

> +	},
> +#endif
>  #ifdef CONFIG_ROCKCHIP_ERRATUM_3588001
>  	{
>  		.desc   = "ITS: Rockchip erratum RK3588001",

If the HW is this bad, and that this only affects virtual interrupts,
why don't you simply disable GICv4 support? Messing with the internals
of the ITS tables feels like a pretty bad idea. Or as I suggested
above, get the firmware to ignore RAS events from the ITS.

	M.

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