linux-kernel - Re: [RFC 1/1] irqchip/gic-v3: Add RK3588 GICR and GITS no share workaround

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <8df79b4a-c7fb-3483-0d95-a97876f92f1b@arm.com>
Date:   Tue, 28 Feb 2023 17:21:57 +0000
From:   Robin Murphy <robin.murphy@....com>
To:     Lucas Tanure <lucas.tanure@...labora.com>,
        Catalin Marinas <catalin.marinas@....com>,
        Will Deacon <will@...nel.org>,
        Jonathan Corbet <corbet@....net>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Peter Geis <pgwipeout@...il.com>,
        Kever Yang <kever.yang@...k-chips.com>
Cc:     linux-rockchip@...ts.infradead.org,
        linux-arm-kernel@...ts.infradead.org, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, kernel@...labora.com
Subject: Re: [RFC 1/1] irqchip/gic-v3: Add RK3588 GICR and GITS no share
 workaround

On 27/02/2023 3:18 pm, Lucas Tanure wrote:
> The GIC600 integration in RK356x, used in rk3588, doesn't support
> any of the shareability or cacheability attributes, and requires
> both values to be set to 0b00 for all the ITS and Redistributor
> tables.
> 
> Based on work of Peter Geis for the Quartz64 board.
> 
> Signed-off-by: Lucas Tanure <lucas.tanure@...labora.com>
> ---
>   Documentation/arm64/silicon-errata.rst |  4 +++
>   arch/arm64/Kconfig                     | 13 ++++++++
>   drivers/irqchip/irq-gic-v3-its.c       | 42 ++++++++++++++++++++++++++
>   3 files changed, 59 insertions(+)
> 
> diff --git a/Documentation/arm64/silicon-errata.rst b/Documentation/arm64/silicon-errata.rst
> index ec5f889d7681..b26cf8ca7d5c 100644
> --- a/Documentation/arm64/silicon-errata.rst
> +++ b/Documentation/arm64/silicon-errata.rst
> @@ -209,3 +209,7 @@ stable kernels.
>   +----------------+-----------------+-----------------+-----------------------------+
>   | Fujitsu        | A64FX           | E#010001        | FUJITSU_ERRATUM_010001      |
>   +----------------+-----------------+-----------------+-----------------------------+
> +
> ++----------------+-----------------+-----------------+-----------------------------+
> +| RockChip       | RK3588          | N/A             | ROCKCHIP_NO_SHARE           |
> ++----------------+-----------------+-----------------+-----------------------------+
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 27b2592698b0..ad3f1742052b 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -1150,6 +1150,19 @@ config NVIDIA_CARMEL_CNP_ERRATUM
>   
>   	  If unsure, say Y.
>   
> +config ROCKCHIP_NO_SHARE
> +	bool "Rockchip RK3588 GIC6000 No shareability or cacheability attributes"
> +	default y
> +	help
> +	  The GIC600 integration in RK356x doesn't support any of the shareability or
> +	  cacheability attributes, and requires both values to be set to 0b00 for all
> +	  the ITS and Redistributor tables.
> +
> +	  Work around the issue by clearing the GICR_PROPBASER_SHAREABILITY_MASK from
> +	  register reads at GICR and GITS.
> +
> +	  If unsure, say Y.
> +
>   config SOCIONEXT_SYNQUACER_PREITS
>   	bool "Socionext Synquacer: Workaround for GICv3 pre-ITS"
>   	default y
> diff --git a/drivers/irqchip/irq-gic-v3-its.c b/drivers/irqchip/irq-gic-v3-its.c
> index 586271b8aa39..637e2e2a1ab1 100644
> --- a/drivers/irqchip/irq-gic-v3-its.c
> +++ b/drivers/irqchip/irq-gic-v3-its.c
> @@ -42,6 +42,7 @@
>   #define ITS_FLAGS_CMDQ_NEEDS_FLUSHING		(1ULL << 0)
>   #define ITS_FLAGS_WORKAROUND_CAVIUM_22375	(1ULL << 1)
>   #define ITS_FLAGS_WORKAROUND_CAVIUM_23144	(1ULL << 2)
> +#define ITS_FLAGS_WORKAROUND_ROCKCHIP_NOSHARE	(1ULL << 3)
>   
>   #define RDIST_FLAGS_PROPBASE_NEEDS_FLUSHING	(1 << 0)
>   #define RDIST_FLAGS_RD_TABLES_PREALLOCATED	(1 << 1)
> @@ -2359,6 +2360,15 @@ static int its_setup_baser(struct its_node *its, struct its_baser *baser,
>   	its_write_baser(its, baser, val);
>   	tmp = baser->val;
>   
> +#if CONFIG_ROCKCHIP_NO_SHARE
> +	if (its->flags & ITS_FLAGS_WORKAROUND_ROCKCHIP_NOSHARE) {
> +		if (tmp & GITS_BASER_SHAREABILITY_MASK)
> +			tmp &= ~GITS_BASER_SHAREABILITY_MASK;
> +		else
> +			gic_flush_dcache_to_poc(base, PAGE_ORDER_TO_SIZE(order));
> +	}
> +#endif
> +
>   	if ((val ^ tmp) & GITS_BASER_SHAREABILITY_MASK) {
>   		/*
>   		 * Shareability didn't stick. Just use
> @@ -3057,6 +3067,7 @@ static void its_cpu_init_lpis(void)
>   {
>   	void __iomem *rbase = gic_data_rdist_rd_base();
>   	struct page *pend_page;
> +	struct its_node *its;
>   	phys_addr_t paddr;
>   	u64 val, tmp;
>   
> @@ -3096,6 +3107,12 @@ static void its_cpu_init_lpis(void)
>   	gicr_write_propbaser(val, rbase + GICR_PROPBASER);
>   	tmp = gicr_read_propbaser(rbase + GICR_PROPBASER);
>   
> +#if CONFIG_ROCKCHIP_NO_SHARE
> +	its = list_first_entry(&its_nodes, struct its_node, entry);
> +	if (its->flags & ITS_FLAGS_WORKAROUND_ROCKCHIP_NOSHARE)
> +		tmp &= ~GICR_PROPBASER_SHAREABILITY_MASK;
> +#endif
> +
>   	if ((tmp ^ val) & GICR_PROPBASER_SHAREABILITY_MASK) {
>   		if (!(tmp & GICR_PROPBASER_SHAREABILITY_MASK)) {
>   			/*
> @@ -3120,6 +3137,11 @@ static void its_cpu_init_lpis(void)
>   	gicr_write_pendbaser(val, rbase + GICR_PENDBASER);
>   	tmp = gicr_read_pendbaser(rbase + GICR_PENDBASER);
>   
> +#if CONFIG_ROCKCHIP_NO_SHARE
> +	if (its->flags & ITS_FLAGS_WORKAROUND_ROCKCHIP_NOSHARE)
> +		tmp &= ~GICR_PENDBASER_SHAREABILITY_MASK;
> +#endif
> +
>   	if (!(tmp & GICR_PENDBASER_SHAREABILITY_MASK)) {
>   		/*
>   		 * The HW reports non-shareable, we must remove the
> @@ -4710,6 +4732,14 @@ static bool __maybe_unused its_enable_quirk_hip07_161600802(void *data)
>   	return true;
>   }
>   
> +static bool __maybe_unused its_enable_quirk_rk356x(void *data)
> +{
> +	struct its_node *its = data;
> +
> +	its->flags |= ITS_FLAGS_WORKAROUND_ROCKCHIP_NOSHARE;
> +	return true;
> +}
> +
>   static const struct gic_quirk its_quirks[] = {
>   #ifdef CONFIG_CAVIUM_ERRATUM_22375
>   	{
> @@ -4755,6 +4785,14 @@ static const struct gic_quirk its_quirks[] = {
>   		.mask	= 0xffffffff,
>   		.init	= its_enable_quirk_hip07_161600802,
>   	},
> +#endif
> +#ifdef CONFIG_ROCKCHIP_NO_SHARE
> +	{
> +		.desc	= "ITS: Rockchip RK356X/RK3588 doesn't support shareability",
> +		.iidr	= 0x0201743b,

This represents the Arm Ltd. GIC-600 implementation. It is definitely 
not Rockchip-specific, and applying this quirk to the likes of Ampere 
Altra or AWS Graviton2 would be extremely unpopular.

TBH I think this whole thing would be reasonable to handle in a generic 
manner using the now-standard "dma-noncoherent" property to override the 
driver's expectations. Given the apparent lack of clear integration 
guidelines it's only likely to continue happening.

Thanks,
Robin.

> +		.mask	= 0xffffffff,
> +		.init	= its_enable_quirk_rk356x,
> +	},
>   #endif
>   	{
>   	}
> @@ -5096,6 +5134,10 @@ static int __init its_probe_one(struct resource *res,
>   	gits_write_cbaser(baser, its->base + GITS_CBASER);
>   	tmp = gits_read_cbaser(its->base + GITS_CBASER);
>   
> +#if CONFIG_ROCKCHIP_NO_SHARE
> +	if (its->flags & ITS_FLAGS_WORKAROUND_ROCKCHIP_NOSHARE)
> +		tmp &= ~GITS_CBASER_SHAREABILITY_MASK;
> +#endif
>   	if ((tmp ^ baser) & GITS_CBASER_SHAREABILITY_MASK) {
>   		if (!(tmp & GITS_CBASER_SHAREABILITY_MASK)) {
>   			/*