Date:   Thu, 16 Nov 2017 14:40:40 +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  2:24:31 pm GMT, "Liuwenliang (Abbott Liu)" <liuwenliang@...wei.com> wrote:
> On 16/11/17  17:54 Marc Zyngier [mailto:marc.zyngier@....com] wrote:
>>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 for your reviews.
> Yes, you are right. I have tested that "mcrr/mrrc" instruction
> (__ACCESS_CP15_64) can work on non LPAE on vexpress_a9.

No, it doesn't. It cannot work, because Cortex-A9 predates the invention
of the 64bit accessor. I suspect that you are testing stuff in QEMU,
which is giving you a SW model that always supports LPAE. I suggest you
test this code on *real* HW, and not only on QEMU.

What I have said is:

- If the CPU supports LPAE, then both 32 and 64bit accessors work
- If the CPU doesn't support LPAE, then only the 32bit accssor work
- In both cases, that's a function of the CPU, and not of the kernel
  configuration.
- Both accessors can be safely defined as long as we ensure that they
  are used in the right context.

> Here is the code I tested on vexpress_a9 and vexpress_a15:
> --- a/arch/arm/include/asm/cp15.h
> +++ b/arch/arm/include/asm/cp15.h
> @@ -64,6 +64,56 @@
>  #define __write_sysreg(v, r, w, c, t)  asm volatile(w " " c : : "r" ((t)(v)))
>  #define write_sysreg(v, ...)           __write_sysreg(v, __VA_ARGS__)
>
> +#define TTBR0           __ACCESS_CP15_64(0, c2)
> +#define TTBR1           __ACCESS_CP15_64(1, c2)
> +#define PAR             __ACCESS_CP15_64(0, c7)

You still need to add the 32bit accessors.

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

Powered by blists - more mailing lists