Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 28 Nov 2019 14:51:51 +0000
From:   Mark Rutland <mark.rutland@arm.com>
To:     Pavel Tatashin <pasha.tatashin@soleen.com>
Cc:     jmorris@namei.org, sashal@kernel.org, linux-kernel@vger.kernel.org,
        catalin.marinas@arm.com, will@kernel.org, steve.capper@arm.com,
        linux-arm-kernel@lists.infradead.org, marc.zyngier@arm.com,
        james.morse@arm.com, vladimir.murzin@arm.com, tglx@linutronix.de,
        gregkh@linuxfoundation.org, allison@lohutok.net, info@metux.net,
        alexios.zavras@intel.com, sstabellini@kernel.org,
        boris.ostrovsky@oracle.com, jgross@suse.com, stefan@agner.ch,
        yamada.masahiro@socionext.com, xen-devel@lists.xenproject.org,
        linux@armlinux.org.uk
Subject: Re: [PATCH 2/3] arm64: remove uaccess_ttbr0 asm macros from cache
 functions
Message-ID: <20191128145151.GB22314@lakrids.cambridge.arm.com>
References: <20191127184453.229321-1-pasha.tatashin@soleen.com>
 <20191127184453.229321-3-pasha.tatashin@soleen.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20191127184453.229321-3-pasha.tatashin@soleen.com>
User-Agent: Mutt/1.11.1+11 (2f07cb52) (2018-12-01)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Nov 27, 2019 at 01:44:52PM -0500, Pavel Tatashin wrote:
> We currently duplicate the logic to enable/disable uaccess via TTBR0,
> with C functions and assembly macros. This is a maintenenace burden
> and is liable to lead to subtle bugs, so let's get rid of the assembly
> macros, and always use the C functions. This requires refactoring
> some assembly functions to have a C wrapper.

[...]

> +static inline int invalidate_icache_range(unsigned long start,
> +					  unsigned long end)
> +{
> +	int rv;

Please make this 'ret', for consistency with other arm64 code. We only
use 'rv' in one place where it's short for "Revision and Variant", and
'ret' is our usual name for a temporary variable used to hold a return
value.

> +
> +	if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
> +		isb();
> +		return 0;
> +	}
> +	uaccess_ttbr0_enable();

Please place a newline between these two, for consistency with other
arm64 code.

> +	rv = asm_invalidate_icache_range(start, end);
> +	uaccess_ttbr0_disable();
> +
> +	return rv;
> +}
> +
>  static inline void flush_icache_range(unsigned long start, unsigned long end)
>  {
>  	__flush_icache_range(start, end);
> diff --git a/arch/arm64/mm/cache.S b/arch/arm64/mm/cache.S
> index db767b072601..a48b6dba304e 100644
> --- a/arch/arm64/mm/cache.S
> +++ b/arch/arm64/mm/cache.S
> @@ -15,7 +15,7 @@
>  #include <asm/asm-uaccess.h>
>  
>  /*
> - *	flush_icache_range(start,end)
> + *	__asm_flush_icache_range(start,end)
>   *
>   *	Ensure that the I and D caches are coherent within specified region.
>   *	This is typically used when code has been written to a memory region,
> @@ -24,11 +24,11 @@
>   *	- start   - virtual start address of region
>   *	- end     - virtual end address of region
>   */
> -ENTRY(__flush_icache_range)
> +ENTRY(__asm_flush_icache_range)
>  	/* FALLTHROUGH */
>  
>  /*
> - *	__flush_cache_user_range(start,end)
> + *	__asm_flush_cache_user_range(start,end)
>   *
>   *	Ensure that the I and D caches are coherent within specified region.
>   *	This is typically used when code has been written to a memory region,
> @@ -37,8 +37,7 @@ ENTRY(__flush_icache_range)
>   *	- start   - virtual start address of region
>   *	- end     - virtual end address of region
>   */
> -ENTRY(__flush_cache_user_range)
> -	uaccess_ttbr0_enable x2, x3, x4
> +ENTRY(__asm_flush_cache_user_range)
>  alternative_if ARM64_HAS_CACHE_IDC

It's unfortunate that we pulled the IDC alternative out for
invalidate_icache_range(), but not here.

If we want to pull out that, then I reckon what we might want to do is
have two asm primitives:

* __asm_clean_dcache_range
* __asm_invalidate_icache_range

... which just do the by_line op, with a fixup that can return -EFAULT.

Then we can give each a C wrapper that just does the IDC/DIC check, e.g.

static int __clean_dcache_range(unsigned long start, unsigned long end)
{
	if (cpus_have_const_cap(ARM64_HAS_CACHE_IDC)) {
		dsb(ishst);
		return 0;
	}

	return __asm_clean_dcache_range(start, end);
}

Then we can build all the more complicated variants atop of those, e.g.

static int __flush_cache_user_range(unsigned long start, unsigned long end)
{
	int ret;

	uaccess_ttbr0_enable();

	ret = __clean_dcache_range(start, end);
	if (ret)
		goto out;
	
	ret = __invalidate_icache_range(start, end);

out:
	uaccess_ttbr0_disable();
	return ret;
}

Thanks,
Mark.