Re: [Xen-devel] [PATCH v2 2/3] arm64: remove uaccess_ttbr0 asm macros from cache functions

[Mark Rutland <mark.rutland@xxxxxxx>]

Hi Pavel,

On Thu, Nov 21, 2019 at 09:24:05PM -0500, Pavel Tatashin wrote:
> Replace the uaccess_ttbr0_disable/uaccess_ttbr0_enable via
> inline variants, and remove asm macros.

A commit message should provide rationale, rather than just a
description of the patch. Something like:

| 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;
> +#if ARM64_HAS_CACHE_DIC
> +     rv = arch_invalidate_icache_range(start, end);
> +#else
> +     uaccess_ttbr0_enable();
> +     rv = arch_invalidate_icache_range(start, end);
> +     uaccess_ttbr0_disable();
> +#endif
> +     return rv;
> +}

This ifdeffery is not the same as an alternative_if, and even if it were
the ARM64_HAS_CACHE_DIC behaviour is not the same as the existing
assembly.

This should be:

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

        if (cpus_have_const_cap(ARM64_HAS_CACHE_DIC)) {
                isb();
                return 0;
        }
        
        uaccess_ttbr0_enable();
        ret = arch_invalidate_icache_range(start, end);
        uaccess_ttbr0_disable();

        return ret;
}

The 'arch_' prefix should probably be 'asm_' (or have an '_asm' suffix),
since this is entirely local to the arch code, and even then should only
be called from the C wrappers.

Thanks,
Mark.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel