Date:   Thu, 16 Nov 2017 09:54:23 +0000
From:   Marc Zyngier <marc.zyngier@....com>
To:     "Liuwenliang \(Abbott Liu\)" <liuwenliang@...wei.com>
Cc:     "linux\@armlinux.org.uk" <linux@...linux.org.uk>,
        "aryabinin\@virtuozzo.com" <aryabinin@...tuozzo.com>,
        "afzal.mohd.ma\@gmail.com" <afzal.mohd.ma@...il.com>,
        "f.fainelli\@gmail.com" <f.fainelli@...il.com>,
        "labbott\@redhat.com" <labbott@...hat.com>,
        "kirill.shutemov\@linux.intel.com" <kirill.shutemov@...ux.intel.com>,
        "mhocko\@suse.com" <mhocko@...e.com>,
        "cdall\@linaro.org" <cdall@...aro.org>,
        "catalin.marinas\@arm.com" <catalin.marinas@....com>,
        "akpm\@linux-foundation.org" <akpm@...ux-foundation.org>,
        "mawilcox\@microsoft.com" <mawilcox@...rosoft.com>,
        "tglx\@linutronix.de" <tglx@...utronix.de>,
        "thgarnie\@google.com" <thgarnie@...gle.com>,
        "keescook\@chromium.org" <keescook@...omium.org>,
        "arnd\@arndb.de" <arnd@...db.de>,
        "vladimir.murzin\@arm.com" <vladimir.murzin@....com>,
        "tixy\@linaro.org" <tixy@...aro.org>,
        "ard.biesheuvel\@linaro.org" <ard.biesheuvel@...aro.org>,
        "robin.murphy\@arm.com" <robin.murphy@....com>,
        "mingo\@kernel.org" <mingo@...nel.org>,
        "grygorii.strashko\@linaro.org" <grygorii.strashko@...aro.org>,
        "glider\@google.com" <glider@...gle.com>,
        "dvyukov\@google.com" <dvyukov@...gle.com>,
        "opendmb\@gmail.com" <opendmb@...il.com>,
        "linux-arm-kernel\@lists.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
        "kasan-dev\@googlegroups.com" <kasan-dev@...glegroups.com>,
        "linux-mm\@kvack.org" <linux-mm@...ck.org>,
        Jiazhenghua <jiazhenghua@...wei.com>,
        Dailei <dylix.dailei@...wei.com>,
        Zengweilin <zengweilin@...wei.com>,
        Heshaoliang <heshaoliang@...wei.com>
Subject: Re: [PATCH 01/11] Initialize the mapping of KASan shadow memory

On Thu, Nov 16 2017 at  3:07:54 am GMT, "Liuwenliang (Abbott Liu)" <liuwenliang@...wei.com> wrote:
>>On 15/11/17 13:16, Liuwenliang (Abbott Liu) wrote:
>>> On 09/11/17  18:36 Marc Zyngier [mailto:marc.zyngier@....com] wrote:
>>>> On Wed, Nov 15 2017 at 10:20:02 am GMT, "Liuwenliang (Abbott Liu)"
>>>> <liuwenliang@...wei.com> wrote:
>>>>> diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
>>>>> index dbdbce1..6db1f51 100644
>>>>> --- a/arch/arm/include/asm/cp15.h
>>>>> +++ b/arch/arm/include/asm/cp15.h
>>>>> @@ -64,6 +64,43 @@
>>>>>  #define __write_sysreg(v, r, w, c, t) asm volatile(w " " c : :
>>>>> "r" ((t)(v)))
>>>>>  #define write_sysreg(v, ...)           __write_sysreg(v, __VA_ARGS__)
>>>>>
>>>>> +#ifdef CONFIG_ARM_LPAE
>>>>> +#define TTBR0           __ACCESS_CP15_64(0, c2)
>>>>> +#define TTBR1           __ACCESS_CP15_64(1, c2)
>>>>> +#define PAR             __ACCESS_CP15_64(0, c7)
>>>>> +#else
>>>>> +#define TTBR0           __ACCESS_CP15(c2, 0, c0, 0)
>>>>> +#define TTBR1           __ACCESS_CP15(c2, 0, c0, 1)
>>>>> +#define PAR             __ACCESS_CP15(c7, 0, c4, 0)
>>>>> +#endif
>>>> Again: there is no point in not having these register encodings
>>>> cohabiting. They are both perfectly defined in the architecture. Just
>>>> suffix one (or even both) with their respective size, making it obvious
>>>> which one you're talking about.
>>> 
>>> I am sorry that I didn't point why I need to define TTBR0/
>>> TTBR1/PAR in to different way
>>> between CONFIG_ARM_LPAE and non CONFIG_ARM_LPAE.
>>> The following description is the reason:
>>> Here is the description come from
>>> DDI0406C2c_arm_architecture_reference_manual.pdf:
>>[...]
>>
>>You're missing the point. TTBR0 existence as a 64bit CP15 register has
>>nothing to do the kernel being compiled with LPAE or not. It has
>>everything to do with the HW supporting LPAE, and it is the kernel's job
>>to use the right accessor depending on how it is compiled. On a CPU
>>supporting LPAE, both TTBR0 accessors are valid. It is the kernel that
>>chooses to use one rather than the other.
>
> Thanks for your review.  I don't think both TTBR0 accessors(64bit
> accessor and 32bit accessor) are valid on a CPU supporting LPAE which
> the LPAE is enabled. Here is the description come form
> DDI0406C2c_arm_architecture_reference_manual.pdf (=ARM® Architecture
> Reference Manual ARMv7-A and ARMv7-R edition) which you can get the
> document by google "ARM® Architecture Reference Manual ARMv7-A and
> ARMv7-R edition".

Trust me, from where I seat, I have a much better source than Google for
that document. Who would have thought?

Nothing in what you randomly quote invalids what I've been saying. And
to show you what's wrong with your reasoning, let me describe a
scenario,

I have a non-LPAE kernel that runs on my system. It uses the 32bit
version of the TTBRs. It turns out that this kernel runs under a
hypervisor (KVM, Xen, or your toy of the day). The hypervisor
context-switches vcpus without even looking at whether the configuration
of that guest. It doesn't have to care. It just blindly uses the 64bit
version of the TTBRs.

The architecture *guarantees* that it works (it even works with a 32bit
guest under a 64bit hypervisor). In your world, this doesn't work. I
guess the architecture wins.

> So, I think if you access TTBR0/TTBR1 on CPU supporting LPAE, you must
> use "mcrr/mrrc" instruction (__ACCESS_CP15_64). If you access
> TTBR0/TTBR1 on CPU supporting LPAE by "mcr/mrc" instruction which is
> 32bit version (__ACCESS_CP15), even if the CPU doesn't report error,
> you also lose the high or low 32bit of the TTBR0/TTBR1.

It is not about "supporting LPAE". It is about using the accessor that
makes sense in a particular context. Yes, the architecture allows you to
do something stupid. Don't do it. It doesn't mean the accessors cannot
be used, and I hope that my example above demonstrates it.

Conclusion: I still stand by my request that both versions of TTBRs/PAR
are described without depending on the kernel configuration, because
this has nothing to do with the kernel configuration.

Thanks,

	M.
-- 
Jazz is not dead, it just smell funny.

Powered by blists - more mailing lists