Re: [PATCH 01/11] Initialize the mapping of KASan shadow memory

[Marc Zyngier]

On Thu, Nov 16 2017 at  2:24:31 pm GMT, "Liuwenliang (Abbott Liu)" <liuwenliang@xxxxxxxxxx> wrote:
> On 16/11/17  17:54 Marc Zyngier [mailto:marc.zyngier@xxxxxxx] wrote:
>>On Thu, Nov 16 2017 at 3:07:54 am GMT, "Liuwenliang (Abbott Liu)"
>> <liuwenliang@xxxxxxxxxx> wrote:
>>>>On 15/11/17 13:16, Liuwenliang (Abbott Liu) wrote:
>>>>> On 09/11/17  18:36 Marc Zyngier [mailto:marc.zyngier@xxxxxxx] wrote:
>>>>>> On Wed, Nov 15 2017 at 10:20:02 am GMT, "Liuwenliang (Abbott Liu)"
>>>>>> <liuwenliang@xxxxxxxxxx> 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.