Re: [PATCH 1/5] arm64: mm: Add slow_virt_to_phys()

[Mark Rutland]

Hi,

On Tue, Jun 19, 2018 at 02:23:11PM -0700, Michael Kelley wrote:
> Add slow_virt_to_phys() function for ARM64 that parallels the same
> function on x86/x64. This is needed by the architecture independent
> Hyper-V VMbus driver at drivers/hv/channel.c.  The implementation
> directly translates the virtual address using the ARM64 'at'
> instruction.
> 
> Signed-off-by: Michael Kelley <mikelley@xxxxxxxxxxxxx>
> Reviewed-by: James Morris <james.morris@xxxxxxxxxxxxx>
> ---
>  arch/arm64/include/asm/memory.h |  6 ++++++
>  arch/arm64/mm/pageattr.c        | 37 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 43 insertions(+)
> 
> diff --git a/arch/arm64/include/asm/memory.h b/arch/arm64/include/asm/memory.h
> index 49d9921..3f68dcf 100644
> --- a/arch/arm64/include/asm/memory.h
> +++ b/arch/arm64/include/asm/memory.h
> @@ -265,6 +265,12 @@ static inline void *phys_to_virt(phys_addr_t x)
>  }
>  
>  /*
> + * For memory where the underlying physical pages may not
> + * be contiguous.
> + */
> +extern phys_addr_t slow_virt_to_phys(void *vaddr);
> +
> +/*
>   * Drivers should NOT use these either.
>   */
>  #define __pa(x)			__virt_to_phys((unsigned long)(x))
> diff --git a/arch/arm64/mm/pageattr.c b/arch/arm64/mm/pageattr.c
> index a563593..8a42cac 100644
> --- a/arch/arm64/mm/pageattr.c
> +++ b/arch/arm64/mm/pageattr.c
> @@ -19,6 +19,8 @@
>  #include <asm/pgtable.h>
>  #include <asm/set_memory.h>
>  #include <asm/tlbflush.h>
> +#include <asm/sysreg.h>
> +#include <asm/barrier.h>
>  
>  struct page_change_data {
>  	pgprot_t set_mask;
> @@ -185,3 +187,38 @@ bool kernel_page_present(struct page *page)
>  }
>  #endif /* CONFIG_HIBERNATION */
>  #endif /* CONFIG_DEBUG_PAGEALLOC */
> +
> +/*
> + * For virtual addresses where the underlyine physical memory may not be
> + * contiguous and the normal virt_to_phys gives the wrong result. This
> + * function does an actual translation using the 'at' instruction.
> + */
> +phys_addr_t slow_virt_to_phys(void *virt_addr)
> +{
> +	phys_addr_t	result;
> +	unsigned long	input = (unsigned long)virt_addr;
> +	char		touch;
> +	int		i;
> +
> +	/* Try up to 3 times (an arbitrary number) */
> +	for (i = 0; i < 3; i++) {
> +		/* Do the translation and check that it worked */
> +		asm volatile("at s1e1r, %0" : : "r" (input));
> +		isb();
> +		result = read_sysreg(par_el1);
> +		if (likely(!(result & 0x1)))
> +			return (result & GENMASK_ULL(51, 12)) |
> +				(input & GENMASK_ULL(11, 0));
> +		/*
> +		 * Something failed. Read the page to fault in anything
> +		 * that isn't resident, then try again. "Anything"
> +		 * could include the page itself or hypervisor page tables.
> +		 */
> +		touch = READ_ONCE(*(char *)input);
> +		dmb(sy);
> +	}
> +
> +	/* Let the caller sort it out. */
> +	return  -1;

AFAICT, callers of slow_virt_to_phys() don't check the value, so this doesn't
seem safe, and I'm concerned by the possibility of spurious failures given we
only retry a fixed number of times.

I think it would be better to walk the page tables, as x86 does, as that's
guaranteed to complete within a fixed bound (i.e. after we read all the levls
of table).

Thanks,
Mark.