Re: [PATCH v2 0/7] KASan for arm

[Liuwenliang (Abbott Liu)]

On 03/20/2018 2:30 AM, Abbott Liu wrote:
>BTW, it looks like you have some section mismatches:
>
>WARNING: vmlinux.o(.meminit.text+0x40): Section mismatch in reference
>from the function kasan_pte_populate() to the function
>.init.text:kasan_alloc_block.constprop.5()
>The function __meminit kasan_pte_populate() references
>a function __init kasan_alloc_block.constprop.5().
>If kasan_alloc_block.constprop.5 is only used by kasan_pte_populate then
>annotate kasan_alloc_block.constprop.5 with a matching annotation.
>
>WARNING: vmlinux.o(.meminit.text+0x144): Section mismatch in reference
>from the function kasan_pmd_populate() to the function
>.init.text:kasan_alloc_block.constprop.5()
>The function __meminit kasan_pmd_populate() references
>a function __init kasan_alloc_block.constprop.5().
>If kasan_alloc_block.constprop.5 is only used by kasan_pmd_populate then
>annotate kasan_alloc_block.constprop.5 with a matching annotation.
>
>WARNING: vmlinux.o(.meminit.text+0x1a4): Section mismatch in reference
>from the function kasan_pud_populate() to the function
>.init.text:kasan_alloc_block.constprop.5()
>The function __meminit kasan_pud_populate() references
>a function __init kasan_alloc_block.constprop.5().
>If kasan_alloc_block.constprop.5 is only used by kasan_pud_populate then
>annotate kasan_alloc_block.constprop.5 with a matching annotation.

Thanks for your testing.
I don't know why the compiler on my machine doesn't report this waring.
Could you test again with adding the following code:
liuwenliang@linux:/home/soft_disk/yocto/linux-git/linux> git diff
diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
index d316f37..ae14d19 100644
--- a/arch/arm/mm/kasan_init.c
+++ b/arch/arm/mm/kasan_init.c
@@ -115,7 +115,7 @@ static void __init clear_pgds(unsigned long start,
pmd_clear(pmd_off_k(start));
 }

-pte_t * __meminit kasan_pte_populate(pmd_t *pmd, unsigned long addr, int node)
+pte_t * __init kasan_pte_populate(pmd_t *pmd, unsigned long addr, int node)
 {
pte_t *pte = pte_offset_kernel(pmd, addr);

@@ -132,7 +132,7 @@ pte_t * __meminit kasan_pte_populate(pmd_t *pmd, unsigned 
long addr, int node)
return pte;
 }

-pmd_t * __meminit kasan_pmd_populate(pud_t *pud, unsigned long addr, int node)
+pmd_t * __init kasan_pmd_populate(pud_t *pud, unsigned long addr, int node)
 {
pmd_t *pmd = pmd_offset(pud, addr);

@@ -146,7 +146,7 @@ pmd_t * __meminit kasan_pmd_populate(pud_t *pud, unsigned 
long addr, int node)
return pmd;
 }

-pud_t * __meminit kasan_pud_populate(pgd_t *pgd, unsigned long addr, int node)
+pud_t * __init kasan_pud_populate(pgd_t *pgd, unsigned long addr, int node)
 {
pud_t *pud = pud_offset(pgd, addr);

@@ -161,7 +161,7 @@ pud_t * __meminit kasan_pud_populate(pgd_t *pgd, unsigned 
long addr, int node)
return pud;
 }

-pgd_t * __meminit kasan_pgd_populate(unsigned long addr, int node)
+pgd_t * __init kasan_pgd_populate(unsigned long addr, int node)
 {
pgd_t *pgd = pgd_offset_k(addr);

Re: [PATCH v2 0/7] KASan for arm

[Liuwenliang (Abbott Liu)]

On 03/20/2018 2:30 AM, Abbott Liu wrote:
>BTW, it looks like you have some section mismatches:
>
>WARNING: vmlinux.o(.meminit.text+0x40): Section mismatch in reference
>from the function kasan_pte_populate() to the function
>.init.text:kasan_alloc_block.constprop.5()
>The function __meminit kasan_pte_populate() references
>a function __init kasan_alloc_block.constprop.5().
>If kasan_alloc_block.constprop.5 is only used by kasan_pte_populate then
>annotate kasan_alloc_block.constprop.5 with a matching annotation.
>
>WARNING: vmlinux.o(.meminit.text+0x144): Section mismatch in reference
>from the function kasan_pmd_populate() to the function
>.init.text:kasan_alloc_block.constprop.5()
>The function __meminit kasan_pmd_populate() references
>a function __init kasan_alloc_block.constprop.5().
>If kasan_alloc_block.constprop.5 is only used by kasan_pmd_populate then
>annotate kasan_alloc_block.constprop.5 with a matching annotation.
>
>WARNING: vmlinux.o(.meminit.text+0x1a4): Section mismatch in reference
>from the function kasan_pud_populate() to the function
>.init.text:kasan_alloc_block.constprop.5()
>The function __meminit kasan_pud_populate() references
>a function __init kasan_alloc_block.constprop.5().
>If kasan_alloc_block.constprop.5 is only used by kasan_pud_populate then
>annotate kasan_alloc_block.constprop.5 with a matching annotation.

Thanks for your testing.
I don't know why the compiler on my machine doesn't report this waring.
Could you test again with adding the following code:
liuwenliang@linux:/home/soft_disk/yocto/linux-git/linux> git diff
diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
index d316f37..ae14d19 100644
--- a/arch/arm/mm/kasan_init.c
+++ b/arch/arm/mm/kasan_init.c
@@ -115,7 +115,7 @@ static void __init clear_pgds(unsigned long start,
pmd_clear(pmd_off_k(start));
 }

-pte_t * __meminit kasan_pte_populate(pmd_t *pmd, unsigned long addr, int node)
+pte_t * __init kasan_pte_populate(pmd_t *pmd, unsigned long addr, int node)
 {
pte_t *pte = pte_offset_kernel(pmd, addr);

@@ -132,7 +132,7 @@ pte_t * __meminit kasan_pte_populate(pmd_t *pmd, unsigned 
long addr, int node)
return pte;
 }

-pmd_t * __meminit kasan_pmd_populate(pud_t *pud, unsigned long addr, int node)
+pmd_t * __init kasan_pmd_populate(pud_t *pud, unsigned long addr, int node)
 {
pmd_t *pmd = pmd_offset(pud, addr);

@@ -146,7 +146,7 @@ pmd_t * __meminit kasan_pmd_populate(pud_t *pud, unsigned 
long addr, int node)
return pmd;
 }

-pud_t * __meminit kasan_pud_populate(pgd_t *pgd, unsigned long addr, int node)
+pud_t * __init kasan_pud_populate(pgd_t *pgd, unsigned long addr, int node)
 {
pud_t *pud = pud_offset(pgd, addr);

@@ -161,7 +161,7 @@ pud_t * __meminit kasan_pud_populate(pgd_t *pgd, unsigned 
long addr, int node)
return pud;
 }

-pgd_t * __meminit kasan_pgd_populate(unsigned long addr, int node)
+pgd_t * __init kasan_pgd_populate(unsigned long addr, int node)
 {
pgd_t *pgd = pgd_offset_k(addr);

Re: [PATCH 3/7] Disable instrumentation for some code

[Liuwenliang (Abbott Liu)]

On 19/03/2018 16:38, Marc Zyngier wrote:
>You need to extend this at least to arch/arm/kvm/hyp/Makefile, as the
>KASAN shadow region won't be mapped in HYP. See commit a6cdf1c08cbfe for
>more details (all the arm64 comments in this patch apply to 32bit as well).
Thanks for your review.
I will disable the instrumentation of arch/arm/kvm/hyp in the next version. 
Just like this:
liuwenliang@linux:/home/soft_disk/yocto/linux-git/linux> git diff
diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
index 63d6b40..0a8b500 100644
--- a/arch/arm/kvm/hyp/Makefile
+++ b/arch/arm/kvm/hyp/Makefile
@@ -24,3 +24,7 @@ obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
 obj-$(CONFIG_KVM_ARM_HOST) += switch.o
 CFLAGS_switch.o   += $(CFLAGS_ARMV7VE)
 obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
+
+GCOV_PROFILE   := n
+KASAN_SANITIZE := n
+UBSAN_SANITIZE := n

Re: [PATCH 3/7] Disable instrumentation for some code

[Liuwenliang (Abbott Liu)]

On 19/03/2018 16:38, Marc Zyngier wrote:
>You need to extend this at least to arch/arm/kvm/hyp/Makefile, as the
>KASAN shadow region won't be mapped in HYP. See commit a6cdf1c08cbfe for
>more details (all the arm64 comments in this patch apply to 32bit as well).
Thanks for your review.
I will disable the instrumentation of arch/arm/kvm/hyp in the next version. 
Just like this:
liuwenliang@linux:/home/soft_disk/yocto/linux-git/linux> git diff
diff --git a/arch/arm/kvm/hyp/Makefile b/arch/arm/kvm/hyp/Makefile
index 63d6b40..0a8b500 100644
--- a/arch/arm/kvm/hyp/Makefile
+++ b/arch/arm/kvm/hyp/Makefile
@@ -24,3 +24,7 @@ obj-$(CONFIG_KVM_ARM_HOST) += hyp-entry.o
 obj-$(CONFIG_KVM_ARM_HOST) += switch.o
 CFLAGS_switch.o   += $(CFLAGS_ARMV7VE)
 obj-$(CONFIG_KVM_ARM_HOST) += s2-setup.o
+
+GCOV_PROFILE   := n
+KASAN_SANITIZE := n
+UBSAN_SANITIZE := n

Re: [PATCH v2 0/7] KASan for arm

[Liuwenliang (Abbott Liu)]

On 03/19/2018 09:23 AM, Florian Fainelli wrote:
>On 03/18/2018 06:20 PM, Liuwenliang (Abbott Liu) wrote:
>> On 03/19/2018 03:14 AM, Florian Fainelli wrote:
>>> Thanks for posting these patches! Just FWIW, you cannot quite add
>>> someone's Tested-by for a patch series that was just resubmitted given
>>> the differences with v1. I just gave it a spin on a Cortex-A5 (no LPAE)
>>> and it looks like test_kasan.ko is passing, great job!
>> 
>> I'm sorry.
>> Thanks for your testing very much!
>> I forget to add Tested-by in cover letter patch file. But I have alreadly 
>> added
>> Tested-by in some of following patch. 
>> In the next version I am going to add Tested-by in all patches.
>
>This is not exactly what I meant. When you submit a v2 of your patches,
>you must wait for people to give you their test results. The Tested-by
>applied to v1, and so much has changed it is no longer valid for v2
>unless someone tells you they tested v2. Hope this is clearer.

Ok, I understand now. thank you for your explanation.

Re: [PATCH v2 0/7] KASan for arm

[Liuwenliang (Abbott Liu)]

On 03/19/2018 09:23 AM, Florian Fainelli wrote:
>On 03/18/2018 06:20 PM, Liuwenliang (Abbott Liu) wrote:
>> On 03/19/2018 03:14 AM, Florian Fainelli wrote:
>>> Thanks for posting these patches! Just FWIW, you cannot quite add
>>> someone's Tested-by for a patch series that was just resubmitted given
>>> the differences with v1. I just gave it a spin on a Cortex-A5 (no LPAE)
>>> and it looks like test_kasan.ko is passing, great job!
>> 
>> I'm sorry.
>> Thanks for your testing very much!
>> I forget to add Tested-by in cover letter patch file. But I have alreadly 
>> added
>> Tested-by in some of following patch. 
>> In the next version I am going to add Tested-by in all patches.
>
>This is not exactly what I meant. When you submit a v2 of your patches,
>you must wait for people to give you their test results. The Tested-by
>applied to v1, and so much has changed it is no longer valid for v2
>unless someone tells you they tested v2. Hope this is clearer.

Ok, I understand now. thank you for your explanation.

Re: [PATCH v2 0/7] KASan for arm

[Liuwenliang (Abbott Liu)]

On 03/19/2018 03:14 AM, Florian Fainelli wrote:
>Thanks for posting these patches! Just FWIW, you cannot quite add
>someone's Tested-by for a patch series that was just resubmitted given
>the differences with v1. I just gave it a spin on a Cortex-A5 (no LPAE)
>and it looks like test_kasan.ko is passing, great job!

I'm sorry.
Thanks for your testing very much!
I forget to add Tested-by in cover letter patch file. But I have alreadly added
Tested-by in some of following patch. 
In the next version I am going to add Tested-by in all patches.

Re: [PATCH v2 0/7] KASan for arm

[Liuwenliang (Abbott Liu)]

On 03/19/2018 03:14 AM, Florian Fainelli wrote:
>Thanks for posting these patches! Just FWIW, you cannot quite add
>someone's Tested-by for a patch series that was just resubmitted given
>the differences with v1. I just gave it a spin on a Cortex-A5 (no LPAE)
>and it looks like test_kasan.ko is passing, great job!

I'm sorry.
Thanks for your testing very much!
I forget to add Tested-by in cover letter patch file. But I have alreadly added
Tested-by in some of following patch. 
In the next version I am going to add Tested-by in all patches.

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

[Liuwenliang (Abbott Liu)]

On Oct 19, 2017 at 19:09, Russell King - ARM Linux 
[mailto:li...@armlinux.org.uk] wrote:
>On Wed, Oct 11, 2017 at 04:22:17PM +0800, Abbott Liu wrote:
>> +#else
>> +#define pud_populate(mm,pmd,pte)do { } while (0)
>> +#endif
>
>Please explain this change - we don't have a "pud" as far as the rest of
>the Linux MM layer is concerned, so why do we need it for kasan?
>
>I suspect it comes from the way we wrap up the page tables - where ARM
>does it one way (because it has to) vs the subsequently merged method
>which is completely upside down to what ARMs doing, and therefore is
>totally incompatible and impossible to fit in with our way.

We will use pud_polulate in kasan_populate_zero_shadow function.

>>  obj-$(CONFIG_CACHE_TAUROS2) += cache-tauros2.o
>> +
>> +KASAN_SANITIZE_kasan_init.o:= n
>> +obj-$(CONFIG_KASAN)+= kasan_init.o
>
>Why is this placed in the middle of the cache object listing?

Sorry, I will place this at the end of the arch/arm/mm/Makefile.
>> +
>> +
>>  obj-$(CONFIG_CACHE_UNIPHIER)+= cache-uniphier.o
...

>> +pgd_t * __meminit kasan_pgd_populate(unsigned long addr, int node)
>> +{
>> +pgd_t *pgd = pgd_offset_k(addr);
>> +if (pgd_none(*pgd)) {
>> +void *p = kasan_alloc_block(PAGE_SIZE, node);
>> +if (!p)
>> +return NULL;
>> +pgd_populate(_mm, pgd, p);
>> +}
>> +return pgd;
>> +}

>This all looks wrong - you are aware that on non-LPAE platforms, there
>is only a _two_ level page table - the top level page table is 16K in
>size, and each _individual_ lower level page table is actually 1024
>bytes, but we do some special handling in the kernel to combine two
>together.  It looks to me that you allocate memory for each Linux-
>abstracted page table level whether the hardware needs it or not.

You are right. If non-LPAE platform check if(pgd_none(*pgd)) true,
void *p = kasan_alloc_block(PAGE_SIZE, node) alloc space is not enough.
But the the function kasan_pgd_populate only used in :
Kasan_init-> create_mapping-> kasan_pgd_populate , so when non-LPAE platform
the if (pgd_none(*pgd)) always false.
But I also think change those code is much better :
if (IS_ENABLED(CONFIG_ARM_LPAE)) {
   p = kasan_alloc_block(PAGE_SIZE, node);
} else {
   /* non-LPAE need 16K for first level pagetabe*/
   p = kasan_alloc_block(PAGE_SIZE*4, node);
}

>Is there any reason why the pre-existing "create_mapping()" function
>can't be used, and you've had to rewrite that code here?

Two reason:
1) Here create_mapping can dynamic alloc phys memory space for mapping to 
virtual space 
Which from start to end, but the create_mapping in arch/arm/mm/mmu.c can't.
2) for LPAE, create_mapping need alloc pgd which we need use virtual space 
below 0xc000,
 here create_mapping can alloc pgd, but create_mapping in arch/arm/mm/mmu.c 
can't.

>> +
>> +static int __init create_mapping(unsigned long start, unsigned long end, 
>> int node)
>> +{
>> +unsigned long addr = start;
>> +pgd_t *pgd;
>> +pud_t *pud;
>> +pmd_t *pmd;
>> +pte_t *pte;
>
>A blank line would help between the auto variables and the code of the
>function.

Ok, I will add blank line in new version.
>> +pr_info("populating shadow for %lx, %lx\n", start, end);
>
>Blank line here too please.

Ok, I will add blank line in new version.

>> +for (; addr < end; addr += PAGE_SIZE) {
>> +pgd = kasan_pgd_populate(addr, node);
>> +if (!pgd)
>> +return -ENOMEM;
...
>> +void __init kasan_init(void)
>> +{
>> +struct memblock_region *reg;
>> +u64 orig_ttbr0;
>> +
>> +orig_ttbr0 = cpu_get_ttbr(0);
>> +
>> +#ifdef CONFIG_ARM_LPAE
>> +memcpy(tmp_pmd_table, 
>> pgd_page_vaddr(*pgd_offset_k(KASAN_SHADOW_START)), sizeof(tmp_pmd_table));
>> +memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
>> +set_pgd(_page_table[pgd_index(KASAN_SHADOW_START)], 
>> __pgd(__pa(tmp_pmd_table) | PMD_TYPE_TABLE | L_PGD_SWAPPER));
>> +cpu_set_ttbr0(__pa(tmp_page_table));
>> +#else
>> +memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
>> +cpu_set_ttbr0(__pa(tmp_page_table));
>> +#endif
>> +flush_cache_all();
>> +local_flush_bp_all();
>> +local_flush_tlb_all();

>What are you trying to achieve with all this complexity?  Some comments
>might be useful, especially for those of us who don't know the internals
>of kasan.
OK, I will add some comments in kasan_init function in new version.
...
>> +for_each_memblock(memory, reg) {
>> +void *start = __va(reg->base);
>> +void *end = __va(reg->base + reg->size);
>
>Isn't this going to complain if the translation macro debugging is enabled?

Sorry, I don't what is the translation macro. Can you tell me.

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

[Liuwenliang (Abbott Liu)]

On Oct 19, 2017 at 19:09, Russell King - ARM Linux 
[mailto:li...@armlinux.org.uk] wrote:
>On Wed, Oct 11, 2017 at 04:22:17PM +0800, Abbott Liu wrote:
>> +#else
>> +#define pud_populate(mm,pmd,pte)do { } while (0)
>> +#endif
>
>Please explain this change - we don't have a "pud" as far as the rest of
>the Linux MM layer is concerned, so why do we need it for kasan?
>
>I suspect it comes from the way we wrap up the page tables - where ARM
>does it one way (because it has to) vs the subsequently merged method
>which is completely upside down to what ARMs doing, and therefore is
>totally incompatible and impossible to fit in with our way.

We will use pud_polulate in kasan_populate_zero_shadow function.

>>  obj-$(CONFIG_CACHE_TAUROS2) += cache-tauros2.o
>> +
>> +KASAN_SANITIZE_kasan_init.o:= n
>> +obj-$(CONFIG_KASAN)+= kasan_init.o
>
>Why is this placed in the middle of the cache object listing?

Sorry, I will place this at the end of the arch/arm/mm/Makefile.
>> +
>> +
>>  obj-$(CONFIG_CACHE_UNIPHIER)+= cache-uniphier.o
...

>> +pgd_t * __meminit kasan_pgd_populate(unsigned long addr, int node)
>> +{
>> +pgd_t *pgd = pgd_offset_k(addr);
>> +if (pgd_none(*pgd)) {
>> +void *p = kasan_alloc_block(PAGE_SIZE, node);
>> +if (!p)
>> +return NULL;
>> +pgd_populate(_mm, pgd, p);
>> +}
>> +return pgd;
>> +}

>This all looks wrong - you are aware that on non-LPAE platforms, there
>is only a _two_ level page table - the top level page table is 16K in
>size, and each _individual_ lower level page table is actually 1024
>bytes, but we do some special handling in the kernel to combine two
>together.  It looks to me that you allocate memory for each Linux-
>abstracted page table level whether the hardware needs it or not.

You are right. If non-LPAE platform check if(pgd_none(*pgd)) true,
void *p = kasan_alloc_block(PAGE_SIZE, node) alloc space is not enough.
But the the function kasan_pgd_populate only used in :
Kasan_init-> create_mapping-> kasan_pgd_populate , so when non-LPAE platform
the if (pgd_none(*pgd)) always false.
But I also think change those code is much better :
if (IS_ENABLED(CONFIG_ARM_LPAE)) {
   p = kasan_alloc_block(PAGE_SIZE, node);
} else {
   /* non-LPAE need 16K for first level pagetabe*/
   p = kasan_alloc_block(PAGE_SIZE*4, node);
}

>Is there any reason why the pre-existing "create_mapping()" function
>can't be used, and you've had to rewrite that code here?

Two reason:
1) Here create_mapping can dynamic alloc phys memory space for mapping to 
virtual space 
Which from start to end, but the create_mapping in arch/arm/mm/mmu.c can't.
2) for LPAE, create_mapping need alloc pgd which we need use virtual space 
below 0xc000,
 here create_mapping can alloc pgd, but create_mapping in arch/arm/mm/mmu.c 
can't.

>> +
>> +static int __init create_mapping(unsigned long start, unsigned long end, 
>> int node)
>> +{
>> +unsigned long addr = start;
>> +pgd_t *pgd;
>> +pud_t *pud;
>> +pmd_t *pmd;
>> +pte_t *pte;
>
>A blank line would help between the auto variables and the code of the
>function.

Ok, I will add blank line in new version.
>> +pr_info("populating shadow for %lx, %lx\n", start, end);
>
>Blank line here too please.

Ok, I will add blank line in new version.

>> +for (; addr < end; addr += PAGE_SIZE) {
>> +pgd = kasan_pgd_populate(addr, node);
>> +if (!pgd)
>> +return -ENOMEM;
...
>> +void __init kasan_init(void)
>> +{
>> +struct memblock_region *reg;
>> +u64 orig_ttbr0;
>> +
>> +orig_ttbr0 = cpu_get_ttbr(0);
>> +
>> +#ifdef CONFIG_ARM_LPAE
>> +memcpy(tmp_pmd_table, 
>> pgd_page_vaddr(*pgd_offset_k(KASAN_SHADOW_START)), sizeof(tmp_pmd_table));
>> +memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
>> +set_pgd(_page_table[pgd_index(KASAN_SHADOW_START)], 
>> __pgd(__pa(tmp_pmd_table) | PMD_TYPE_TABLE | L_PGD_SWAPPER));
>> +cpu_set_ttbr0(__pa(tmp_page_table));
>> +#else
>> +memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
>> +cpu_set_ttbr0(__pa(tmp_page_table));
>> +#endif
>> +flush_cache_all();
>> +local_flush_bp_all();
>> +local_flush_tlb_all();

>What are you trying to achieve with all this complexity?  Some comments
>might be useful, especially for those of us who don't know the internals
>of kasan.
OK, I will add some comments in kasan_init function in new version.
...
>> +for_each_memblock(memory, reg) {
>> +void *start = __va(reg->base);
>> +void *end = __va(reg->base + reg->size);
>
>Isn't this going to complain if the translation macro debugging is enabled?

Sorry, I don't what is the translation macro. Can you tell me.

Re: [PATCH 00/11] KASan for arm

[Liuwenliang (Abbott Liu)]

On 2018/2/14  2:41 AM, Florian Fainelli [f.faine...@gmail.com] wrote:
>Hi Abbott,
>
>Are you planning on picking up these patches and sending a second
>version? I would be more than happy to provide test results once you
>have something, this is very useful, thank you!
>-- 
>Florian

I'm sorry to reply you so late. I had a holiday on last few days.
Yes, I will send the second version, maybe on next two weeks.

Re: [PATCH 00/11] KASan for arm

[Liuwenliang (Abbott Liu)]

On 2018/2/14  2:41 AM, Florian Fainelli [f.faine...@gmail.com] wrote:
>Hi Abbott,
>
>Are you planning on picking up these patches and sending a second
>version? I would be more than happy to provide test results once you
>have something, this is very useful, thank you!
>-- 
>Florian

I'm sorry to reply you so late. I had a holiday on last few days.
Yes, I will send the second version, maybe on next two weeks.

Re: [PATCH 06/11] change memory_is_poisoned_16 for aligned error

[Liuwenliang (Abbott Liu)]

On 6 December 2017 at 1:09  Ard Biesheuvel [ard.biesheu...@linaro.org] wrote:
>On 5 December 2017 at 14:19, Liuwenliang (Abbott Liu)
><liuwenli...@huawei.com> wrote:
>> On Nov 23, 2017  20:30  Russell King - ARM Linux 
>> [mailto:li...@armlinux.org.uk]  wrote:
>>>On Thu, Oct 12, 2017 at 11:27:40AM +, Liuwenliang (Lamb) wrote:
>>>> >> - I don't understand why this is necessary.  memory_is_poisoned_16()
>>>> >>   already handles unaligned addresses?
>>>> >>
>>>> >> - If it's needed on ARM then presumably it will be needed on other
>>>> >>   architectures, so CONFIG_ARM is insufficiently general.
>>>> >>
>>>> >> - If the present memory_is_poisoned_16() indeed doesn't work on ARM,
>>>> >>   it would be better to generalize/fix it in some fashion rather than
>>>> >>   creating a new variant of the function.
>>>>
>>>>
>>>> >Yes, I think it will be better to fix the current function rather then
>>>> >have 2 slightly different copies with ifdef's.
>>>> >Will something along these lines work for arm? 16-byte accesses are
>>>> >not too common, so it should not be a performance problem. And
>>>> >probably modern compilers can turn 2 1-byte checks into a 2-byte check
>>>> >where safe (x86).
>>>>
>>>> >static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>>> >{
>>>> >u8 *shadow_addr = (u8 *)kasan_mem_to_shadow((void *)addr);
>>>> >
>>>> >if (shadow_addr[0] || shadow_addr[1])
>>>> >return true;
>>>> >/* Unaligned 16-bytes access maps into 3 shadow bytes. */
>>>> >if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>>>> >return memory_is_poisoned_1(addr + 15);
>>>> >return false;
>>>> >}
>>>>
>>>> Thanks for Andrew Morton and Dmitry Vyukov's review.
>>>> If the parameter addr=0xc008, now in function:
>>>> static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>>> {
>>>>  --- //shadow_addr = (u16 *)(KASAN_OFFSET+0x1801(=0xc008>>3)) 
>>>> is not
>>>>  --- // unsigned by 2 bytes.
>>>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>>>
>>>> /* Unaligned 16-bytes access maps into 3 shadow bytes. */
>>>> if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>>>> return *shadow_addr || memory_is_poisoned_1(addr + 15);
>>>>   //here is going to be error on arm, specially when kernel has 
>>>> not finished yet.
>>>>   //Because the unsigned accessing cause DataAbort Exception which 
>>>> is not
>>>>   //initialized when kernel is starting.
>>>> return *shadow_addr;
>>>> }
>>>>
>>>> I also think it is better to fix this problem.
>>
>>>What about using get_unaligned() ?
>>
>> Thanks for your review.
>>
>> I think it is good idea to use get_unaligned. But ARMv7 support CONFIG_ 
>> HAVE_EFFICIENT_UNALIGNED_ACCESS
>> (arch/arm/Kconfig : select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || 
>> CPU_V6K || CPU_V7) && MMU).
>> So on ARMv7, the code:
>> u16 *shadow_addr = get_unaligned((u16 *)kasan_mem_to_shadow((void *)addr));
>> equals the code:000
>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>
>
>No it does not. The compiler may merge adjacent accesses into ldm or
>ldrd instructions, which do not tolerate misalignment regardless of
>the SCTLR.A bit.
>
>This is actually something we may need to fix for ARM, i.e., drop
>HAVE_EFFICIENT_UNALIGNED_ACCESS altogether, or carefully review the
>way it is used currently.
>
>> On ARMv7, if SCRLR.A is 0, unaligned access is OK.  Here is the description 
>> comes from ARM(r) Architecture Reference
>> Manual ARMv7-A and ARMv7-R edition :
>>
>
>
>Could you *please* stop quoting the ARM ARM at us? People who are
>seeking detailed information like that will know where to find it.
>
>--
>Ard.

Thanks for Ard Biesheuvel's review.
Using get_unaligned does not give us too much benefit, and get_unaligned may 
have some problem.
So it may be better to not use get_unaligned.

Re: [PATCH 06/11] change memory_is_poisoned_16 for aligned error

[Liuwenliang (Abbott Liu)]

On 6 December 2017 at 1:09  Ard Biesheuvel [ard.biesheu...@linaro.org] wrote:
>On 5 December 2017 at 14:19, Liuwenliang (Abbott Liu)
> wrote:
>> On Nov 23, 2017  20:30  Russell King - ARM Linux 
>> [mailto:li...@armlinux.org.uk]  wrote:
>>>On Thu, Oct 12, 2017 at 11:27:40AM +, Liuwenliang (Lamb) wrote:
>>>> >> - I don't understand why this is necessary.  memory_is_poisoned_16()
>>>> >>   already handles unaligned addresses?
>>>> >>
>>>> >> - If it's needed on ARM then presumably it will be needed on other
>>>> >>   architectures, so CONFIG_ARM is insufficiently general.
>>>> >>
>>>> >> - If the present memory_is_poisoned_16() indeed doesn't work on ARM,
>>>> >>   it would be better to generalize/fix it in some fashion rather than
>>>> >>   creating a new variant of the function.
>>>>
>>>>
>>>> >Yes, I think it will be better to fix the current function rather then
>>>> >have 2 slightly different copies with ifdef's.
>>>> >Will something along these lines work for arm? 16-byte accesses are
>>>> >not too common, so it should not be a performance problem. And
>>>> >probably modern compilers can turn 2 1-byte checks into a 2-byte check
>>>> >where safe (x86).
>>>>
>>>> >static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>>> >{
>>>> >u8 *shadow_addr = (u8 *)kasan_mem_to_shadow((void *)addr);
>>>> >
>>>> >if (shadow_addr[0] || shadow_addr[1])
>>>> >return true;
>>>> >/* Unaligned 16-bytes access maps into 3 shadow bytes. */
>>>> >if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>>>> >return memory_is_poisoned_1(addr + 15);
>>>> >return false;
>>>> >}
>>>>
>>>> Thanks for Andrew Morton and Dmitry Vyukov's review.
>>>> If the parameter addr=0xc008, now in function:
>>>> static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>>>> {
>>>>  --- //shadow_addr = (u16 *)(KASAN_OFFSET+0x1801(=0xc008>>3)) 
>>>> is not
>>>>  --- // unsigned by 2 bytes.
>>>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>>>
>>>> /* Unaligned 16-bytes access maps into 3 shadow bytes. */
>>>> if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>>>> return *shadow_addr || memory_is_poisoned_1(addr + 15);
>>>>   //here is going to be error on arm, specially when kernel has 
>>>> not finished yet.
>>>>   //Because the unsigned accessing cause DataAbort Exception which 
>>>> is not
>>>>   //initialized when kernel is starting.
>>>> return *shadow_addr;
>>>> }
>>>>
>>>> I also think it is better to fix this problem.
>>
>>>What about using get_unaligned() ?
>>
>> Thanks for your review.
>>
>> I think it is good idea to use get_unaligned. But ARMv7 support CONFIG_ 
>> HAVE_EFFICIENT_UNALIGNED_ACCESS
>> (arch/arm/Kconfig : select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || 
>> CPU_V6K || CPU_V7) && MMU).
>> So on ARMv7, the code:
>> u16 *shadow_addr = get_unaligned((u16 *)kasan_mem_to_shadow((void *)addr));
>> equals the code:000
>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>
>
>No it does not. The compiler may merge adjacent accesses into ldm or
>ldrd instructions, which do not tolerate misalignment regardless of
>the SCTLR.A bit.
>
>This is actually something we may need to fix for ARM, i.e., drop
>HAVE_EFFICIENT_UNALIGNED_ACCESS altogether, or carefully review the
>way it is used currently.
>
>> On ARMv7, if SCRLR.A is 0, unaligned access is OK.  Here is the description 
>> comes from ARM(r) Architecture Reference
>> Manual ARMv7-A and ARMv7-R edition :
>>
>
>
>Could you *please* stop quoting the ARM ARM at us? People who are
>seeking detailed information like that will know where to find it.
>
>--
>Ard.

Thanks for Ard Biesheuvel's review.
Using get_unaligned does not give us too much benefit, and get_unaligned may 
have some problem.
So it may be better to not use get_unaligned.

Re: [PATCH 06/11] change memory_is_poisoned_16 for aligned error

[Liuwenliang (Abbott Liu)]

On Nov 23, 2017  20:30  Russell King - ARM Linux [mailto:li...@armlinux.org.uk] 
 wrote:
>On Thu, Oct 12, 2017 at 11:27:40AM +, Liuwenliang (Lamb) wrote:
>> >> - I don't understand why this is necessary.  memory_is_poisoned_16()
>> >>   already handles unaligned addresses?
>> >>
>> >> - If it's needed on ARM then presumably it will be needed on other
>> >>   architectures, so CONFIG_ARM is insufficiently general.
>> >>
>> >> - If the present memory_is_poisoned_16() indeed doesn't work on ARM,
>> >>   it would be better to generalize/fix it in some fashion rather than
>> >>   creating a new variant of the function.
>>
>>
>> >Yes, I think it will be better to fix the current function rather then
>> >have 2 slightly different copies with ifdef's.
>> >Will something along these lines work for arm? 16-byte accesses are
>> >not too common, so it should not be a performance problem. And
>> >probably modern compilers can turn 2 1-byte checks into a 2-byte check
>> >where safe (x86).
>>
>> >static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> >{
>> >u8 *shadow_addr = (u8 *)kasan_mem_to_shadow((void *)addr);
>> >
>> >if (shadow_addr[0] || shadow_addr[1])
>> >return true;
>> >/* Unaligned 16-bytes access maps into 3 shadow bytes. */
>> >if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>> >return memory_is_poisoned_1(addr + 15);
>> >return false;
>> >}
>>
>> Thanks for Andrew Morton and Dmitry Vyukov's review.
>> If the parameter addr=0xc008, now in function:
>> static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> {
>>  --- //shadow_addr = (u16 *)(KASAN_OFFSET+0x1801(=0xc008>>3)) is 
>> not
>>  --- // unsigned by 2 bytes.
>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>
>> /* Unaligned 16-bytes access maps into 3 shadow bytes. */
>> if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>> return *shadow_addr || memory_is_poisoned_1(addr + 15);
>>   //here is going to be error on arm, specially when kernel has not 
>> finished yet.
>>   //Because the unsigned accessing cause DataAbort Exception which 
>> is not
>>   //initialized when kernel is starting.
>> return *shadow_addr;
>> }
>>
>> I also think it is better to fix this problem.

>What about using get_unaligned() ?

Thanks for your review.

I think it is good idea to use get_unaligned. But ARMv7 support CONFIG_ 
HAVE_EFFICIENT_UNALIGNED_ACCESS
(arch/arm/Kconfig : select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || 
CPU_V6K || CPU_V7) && MMU).
So on ARMv7, the code:
u16 *shadow_addr = get_unaligned((u16 *)kasan_mem_to_shadow((void *)addr));
equals the code:000
u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);

On ARMv7, if SCRLR.A is 0, unaligned access is OK.  Here is the description 
comes from ARM(r) Architecture Reference
Manual ARMv7-A and ARMv7-R edition :

A3.2.1 Unaligned data access
An ARMv7 implementation must support unaligned data accesses by some load and 
store instructions, as
Table A3-1 shows. Software can set the SCTLR.A bit to control whether a 
misaligned access by one of these
instructions causes an Alignment fault Data abort exception.

Table A3-1 Alignment requirements of load/store instructions
Instructions Alignment check SCTLR.A is 0   
 SCTLR.A is 1

LDRB, LDREXB, LDRBT,
LDRSB, LDRSBT, STRB, None   --
STREXB, STRBT, SWPB, 
TBB 

LDRH, LDRHT, LDRSH, 
LDRSHT, STRH, STRHT,HalfwordUnaligned access
Alignment fault
TBH 

LDREXH, STREXHHalfwordAlignment fault  
Alignment fault

LDR, LDRT, STR, STRT
PUSH, encodings T3 and A2 only Word  Unaligned access   
  Alignment fault
POP, encodings T3 and A2 only

LDREX, STREXWord Alignment fault   
Alignment fault

LDREXD, STREXD DoublewordAlignment fault   
Alignment fault

All forms of LDM and STM,
LDRD, RFE, SRS, STRD, SWP
PUSH, except for encodings 
T3 and A2   Word  Alignment fault   
Alignment fault
POP, except for encodings 
T3 and A2

LDC, LDC2, STC, STC2 Word  Alignment fault  
 Alignment fault

VLDM, VLDR, VPOP,
 VPUSH, VSTM, VSTR Word  Alignment fault   
Alignment fault

VLD1, VLD2, VLD3, VLD4,
 VST1, VST2, VST3, VST4,  Element sizeUnaligned access  
   Alignment fault
 all with standard alignmenta  

VLD1, VLD2, VLD3, VLD4, 
VST1, VST2, VST3, VST4,   As specified by@   Alignment fault 
  Alignment fault  
all with @ specifieda


On ARMv7, the following code can guarantee that if SCRLR.A is 0:
__enable_mmu:
#if 

Re: [PATCH 06/11] change memory_is_poisoned_16 for aligned error

[Liuwenliang (Abbott Liu)]

On Nov 23, 2017  20:30  Russell King - ARM Linux [mailto:li...@armlinux.org.uk] 
 wrote:
>On Thu, Oct 12, 2017 at 11:27:40AM +, Liuwenliang (Lamb) wrote:
>> >> - I don't understand why this is necessary.  memory_is_poisoned_16()
>> >>   already handles unaligned addresses?
>> >>
>> >> - If it's needed on ARM then presumably it will be needed on other
>> >>   architectures, so CONFIG_ARM is insufficiently general.
>> >>
>> >> - If the present memory_is_poisoned_16() indeed doesn't work on ARM,
>> >>   it would be better to generalize/fix it in some fashion rather than
>> >>   creating a new variant of the function.
>>
>>
>> >Yes, I think it will be better to fix the current function rather then
>> >have 2 slightly different copies with ifdef's.
>> >Will something along these lines work for arm? 16-byte accesses are
>> >not too common, so it should not be a performance problem. And
>> >probably modern compilers can turn 2 1-byte checks into a 2-byte check
>> >where safe (x86).
>>
>> >static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> >{
>> >u8 *shadow_addr = (u8 *)kasan_mem_to_shadow((void *)addr);
>> >
>> >if (shadow_addr[0] || shadow_addr[1])
>> >return true;
>> >/* Unaligned 16-bytes access maps into 3 shadow bytes. */
>> >if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>> >return memory_is_poisoned_1(addr + 15);
>> >return false;
>> >}
>>
>> Thanks for Andrew Morton and Dmitry Vyukov's review.
>> If the parameter addr=0xc008, now in function:
>> static __always_inline bool memory_is_poisoned_16(unsigned long addr)
>> {
>>  --- //shadow_addr = (u16 *)(KASAN_OFFSET+0x1801(=0xc008>>3)) is 
>> not
>>  --- // unsigned by 2 bytes.
>> u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);
>>
>> /* Unaligned 16-bytes access maps into 3 shadow bytes. */
>> if (unlikely(!IS_ALIGNED(addr, KASAN_SHADOW_SCALE_SIZE)))
>> return *shadow_addr || memory_is_poisoned_1(addr + 15);
>>   //here is going to be error on arm, specially when kernel has not 
>> finished yet.
>>   //Because the unsigned accessing cause DataAbort Exception which 
>> is not
>>   //initialized when kernel is starting.
>> return *shadow_addr;
>> }
>>
>> I also think it is better to fix this problem.

>What about using get_unaligned() ?

Thanks for your review.

I think it is good idea to use get_unaligned. But ARMv7 support CONFIG_ 
HAVE_EFFICIENT_UNALIGNED_ACCESS
(arch/arm/Kconfig : select HAVE_EFFICIENT_UNALIGNED_ACCESS if (CPU_V6 || 
CPU_V6K || CPU_V7) && MMU).
So on ARMv7, the code:
u16 *shadow_addr = get_unaligned((u16 *)kasan_mem_to_shadow((void *)addr));
equals the code:000
u16 *shadow_addr = (u16 *)kasan_mem_to_shadow((void *)addr);

On ARMv7, if SCRLR.A is 0, unaligned access is OK.  Here is the description 
comes from ARM(r) Architecture Reference
Manual ARMv7-A and ARMv7-R edition :

A3.2.1 Unaligned data access
An ARMv7 implementation must support unaligned data accesses by some load and 
store instructions, as
Table A3-1 shows. Software can set the SCTLR.A bit to control whether a 
misaligned access by one of these
instructions causes an Alignment fault Data abort exception.

Table A3-1 Alignment requirements of load/store instructions
Instructions Alignment check SCTLR.A is 0   
 SCTLR.A is 1

LDRB, LDREXB, LDRBT,
LDRSB, LDRSBT, STRB, None   --
STREXB, STRBT, SWPB, 
TBB 

LDRH, LDRHT, LDRSH, 
LDRSHT, STRH, STRHT,HalfwordUnaligned access
Alignment fault
TBH 

LDREXH, STREXHHalfwordAlignment fault  
Alignment fault

LDR, LDRT, STR, STRT
PUSH, encodings T3 and A2 only Word  Unaligned access   
  Alignment fault
POP, encodings T3 and A2 only

LDREX, STREXWord Alignment fault   
Alignment fault

LDREXD, STREXD DoublewordAlignment fault   
Alignment fault

All forms of LDM and STM,
LDRD, RFE, SRS, STRD, SWP
PUSH, except for encodings 
T3 and A2   Word  Alignment fault   
Alignment fault
POP, except for encodings 
T3 and A2

LDC, LDC2, STC, STC2 Word  Alignment fault  
 Alignment fault

VLDM, VLDR, VPOP,
 VPUSH, VSTM, VSTR Word  Alignment fault   
Alignment fault

VLD1, VLD2, VLD3, VLD4,
 VST1, VST2, VST3, VST4,  Element sizeUnaligned access  
   Alignment fault
 all with standard alignmenta  

VLD1, VLD2, VLD3, VLD4, 
VST1, VST2, VST3, VST4,   As specified by@   Alignment fault 
  Alignment fault  
all with @ specifieda


On ARMv7, the following code can guarantee that if SCRLR.A is 0:
__enable_mmu:
#if 

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

[Liuwenliang (Abbott Liu)]

On Nov 23, 2017  23:22  Russell King - ARM Linux [mailto:li...@armlinux.org.uk] 
 wrote:
>Please pay attention to the project coding style whenever creating code
>for a program.  It doesn't matter what the project coding style is, as
>long as you write your code to match the style that is already there.
>
>For the kernel, that is: tabs not spaces for indentation of code.
>You seem to be using a variable number of spaces for all the new code
>above.
>
>Some of it seems to be your email client thinking it knows better about
>white space - and such behaviours basically makes patches unapplyable.
>See Documentation/process/email-clients.rst for hints about email
>clients.
Thanks for your review.
I'm going to change it in the new version.  

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

[Liuwenliang (Abbott Liu)]

On Nov 23, 2017  23:22  Russell King - ARM Linux [mailto:li...@armlinux.org.uk] 
 wrote:
>Please pay attention to the project coding style whenever creating code
>for a program.  It doesn't matter what the project coding style is, as
>long as you write your code to match the style that is already there.
>
>For the kernel, that is: tabs not spaces for indentation of code.
>You seem to be using a variable number of spaces for all the new code
>above.
>
>Some of it seems to be your email client thinking it knows better about
>white space - and such behaviours basically makes patches unapplyable.
>See Documentation/process/email-clients.rst for hints about email
>clients.
Thanks for your review.
I'm going to change it in the new version.  

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

[Liuwenliang (Abbott Liu)]

t->cp15[c6_DFAR],   DFAR);
write_sysreg(ctxt->cp15[c6_IFAR],   IFAR);
-   write_sysreg(*cp15_64(ctxt, c7_PAR),PAR);
+ write_sysreg(*cp15_64(ctxt, c7_PAR),PAR_64);
write_sysreg(ctxt->cp15[c10_PRRR],  PRRR);
write_sysreg(ctxt->cp15[c10_NMRR],  NMRR);
write_sysreg(ctxt->cp15[c10_AMAIR0],AMAIR0);
diff --git a/arch/arm/kvm/hyp/switch.c b/arch/arm/kvm/hyp/switch.c
index ebd2dd4..4879588 100644
--- a/arch/arm/kvm/hyp/switch.c
+++ b/arch/arm/kvm/hyp/switch.c
@@ -133,12 +133,12 @@ static bool __hyp_text __populate_fault_info(struct 
kvm_vcpu *vcpu)
if (!(hsr & HSR_DABT_S1PTW) && (hsr & HSR_FSC_TYPE) == FSC_PERM) {
u64 par, tmp;

-   par = read_sysreg(PAR);
+ par = read_sysreg(PAR_64);
write_sysreg(far, ATS1CPR);
isb();

-   tmp = read_sysreg(PAR);
-   write_sysreg(par, PAR);
+ tmp = read_sysreg(PAR_64);
+ write_sysreg(par, PAR_64);

if (unlikely(tmp & 1))
return false; /* Translation failed, back to guest */
diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
index 049ee0a..87c86c7 100644
--- a/arch/arm/mm/kasan_init.c
+++ b/arch/arm/mm/kasan_init.c
@@ -203,16 +203,16 @@ void __init kasan_init(void)
u64 orig_ttbr0;
int i;

-   orig_ttbr0 = cpu_get_ttbr(0);
+ orig_ttbr0 = get_ttbr0();

 #ifdef CONFIG_ARM_LPAE
memcpy(tmp_pmd_table, 
pgd_page_vaddr(*pgd_offset_k(KASAN_SHADOW_START)), sizeof(tmp_pmd_table));
memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
set_pgd(_page_table[pgd_index(KASAN_SHADOW_START)], 
__pgd(__pa(tmp_pmd_table) | PMD_TYPE_TABLE | L_PGD_SWAPPER));
-   cpu_set_ttbr0(__pa(tmp_page_table));
+ set_ttbr0(__pa(tmp_page_table));
 #else
memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
-   cpu_set_ttbr0(__pa(tmp_page_table));
+ set_ttbr0((u64)__pa(tmp_page_table));
 #endif
flush_cache_all();
local_flush_bp_all();
@@ -257,7 +257,7 @@ void __init kasan_init(void)
 /*__pgprot(_L_PTE_DEFAULT | L_PTE_DIRTY | 
L_PTE_XN | L_PTE_RDONLY))*/
__pgprot(pgprot_val(PAGE_KERNEL) | 
L_PTE_RDONLY)));
memset(kasan_zero_page, 0, PAGE_SIZE);
-   cpu_set_ttbr0(orig_ttbr0);
+ set_ttbr0(orig_ttbr0);
flush_cache_all();
local_flush_bp_all();
local_flush_tlb_all();



-邮件原件-
发件人: Marc Zyngier [mailto:marc.zyng...@arm.com] 
发送时间: 2017年11月22日 21:06
收件人: Liuwenliang (Abbott Liu); Mark Rutland
抄送: t...@linaro.org; mho...@suse.com; grygorii.stras...@linaro.org; 
catalin.mari...@arm.com; linux...@kvack.org; gli...@google.com; 
afzal.mohd...@gmail.com; mi...@kernel.org; Christoffer Dall; 
f.faine...@gmail.com; mawil...@microsoft.com; li...@armlinux.org.uk; 
kasan-...@googlegroups.com; Dailei; linux-arm-ker...@lists.infradead.org; 
aryabi...@virtuozzo.com; labb...@redhat.com; vladimir.mur...@arm.com; 
keesc...@chromium.org; a...@arndb.de; Zengweilin; open...@gmail.com; 
Heshaoliang; t...@linutronix.de; dvyu...@google.com; ard.biesheu...@linaro.org; 
linux-kernel@vger.kernel.org; Jiazhenghua; a...@linux-foundation.org; 
robin.mur...@arm.com; thgar...@google.com; kirill.shute...@linux.intel.com
主题: Re: [PATCH 01/11] Initialize the mapping of KASan shadow memory

On 22/11/17 12:56, Liuwenliang (Abbott Liu) wrote:
> On Nov 22, 2017  20:30  Mark Rutland [mailto:mark.rutl...@arm.com] wrote:
>> On Tue, Nov 21, 2017 at 07:59:01AM +, Liuwenliang (Abbott Liu) wrote:
>>> On Nov 17, 2017  21:49  Marc Zyngier [mailto:marc.zyng...@arm.com]  wrote:
>>>> On Sat, 18 Nov 2017 10:40:08 +
>>>> "Liuwenliang (Abbott Liu)" <liuwenli...@huawei.com> wrote:
>>>>> On Nov 17, 2017  15:36 Christoffer Dall [mailto:cd...@linaro.org]  wrote:
> 
>>> Please don't ask people to limit to 4GB of physical space on CPU
>>> supporting LPAE, please don't ask people to guaranteed to have some
>>> memory below 4GB on CPU supporting LPAE.
> 
>> I don't think that Marc is suggesting that you'd always use the 32-bit
>> accessors on an LPAE system, just that all the definitions should exist
>> regardless of configuration.
> 
>> So rather than this:
> 
>>> +#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
> 
>> ... you'd have the following in cp15.h:
> 
>> #define TTBR0_64   __ACCESS_CP

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

[Liuwenliang (Abbott Liu)]

t->cp15[c6_DFAR],   DFAR);
write_sysreg(ctxt->cp15[c6_IFAR],   IFAR);
-   write_sysreg(*cp15_64(ctxt, c7_PAR),PAR);
+ write_sysreg(*cp15_64(ctxt, c7_PAR),PAR_64);
write_sysreg(ctxt->cp15[c10_PRRR],  PRRR);
write_sysreg(ctxt->cp15[c10_NMRR],  NMRR);
write_sysreg(ctxt->cp15[c10_AMAIR0],AMAIR0);
diff --git a/arch/arm/kvm/hyp/switch.c b/arch/arm/kvm/hyp/switch.c
index ebd2dd4..4879588 100644
--- a/arch/arm/kvm/hyp/switch.c
+++ b/arch/arm/kvm/hyp/switch.c
@@ -133,12 +133,12 @@ static bool __hyp_text __populate_fault_info(struct 
kvm_vcpu *vcpu)
if (!(hsr & HSR_DABT_S1PTW) && (hsr & HSR_FSC_TYPE) == FSC_PERM) {
u64 par, tmp;

-   par = read_sysreg(PAR);
+ par = read_sysreg(PAR_64);
write_sysreg(far, ATS1CPR);
isb();

-   tmp = read_sysreg(PAR);
-   write_sysreg(par, PAR);
+ tmp = read_sysreg(PAR_64);
+ write_sysreg(par, PAR_64);

if (unlikely(tmp & 1))
return false; /* Translation failed, back to guest */
diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
index 049ee0a..87c86c7 100644
--- a/arch/arm/mm/kasan_init.c
+++ b/arch/arm/mm/kasan_init.c
@@ -203,16 +203,16 @@ void __init kasan_init(void)
u64 orig_ttbr0;
int i;

-   orig_ttbr0 = cpu_get_ttbr(0);
+ orig_ttbr0 = get_ttbr0();

 #ifdef CONFIG_ARM_LPAE
memcpy(tmp_pmd_table, 
pgd_page_vaddr(*pgd_offset_k(KASAN_SHADOW_START)), sizeof(tmp_pmd_table));
memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
set_pgd(_page_table[pgd_index(KASAN_SHADOW_START)], 
__pgd(__pa(tmp_pmd_table) | PMD_TYPE_TABLE | L_PGD_SWAPPER));
-   cpu_set_ttbr0(__pa(tmp_page_table));
+ set_ttbr0(__pa(tmp_page_table));
 #else
memcpy(tmp_page_table, swapper_pg_dir, sizeof(tmp_page_table));
-   cpu_set_ttbr0(__pa(tmp_page_table));
+ set_ttbr0((u64)__pa(tmp_page_table));
 #endif
flush_cache_all();
local_flush_bp_all();
@@ -257,7 +257,7 @@ void __init kasan_init(void)
 /*__pgprot(_L_PTE_DEFAULT | L_PTE_DIRTY | 
L_PTE_XN | L_PTE_RDONLY))*/
__pgprot(pgprot_val(PAGE_KERNEL) | 
L_PTE_RDONLY)));
memset(kasan_zero_page, 0, PAGE_SIZE);
-   cpu_set_ttbr0(orig_ttbr0);
+ set_ttbr0(orig_ttbr0);
flush_cache_all();
local_flush_bp_all();
local_flush_tlb_all();



-邮件原件-
发件人: Marc Zyngier [mailto:marc.zyng...@arm.com] 
发送时间: 2017年11月22日 21:06
收件人: Liuwenliang (Abbott Liu); Mark Rutland
抄送: t...@linaro.org; mho...@suse.com; grygorii.stras...@linaro.org; 
catalin.mari...@arm.com; linux...@kvack.org; gli...@google.com; 
afzal.mohd...@gmail.com; mi...@kernel.org; Christoffer Dall; 
f.faine...@gmail.com; mawil...@microsoft.com; li...@armlinux.org.uk; 
kasan-...@googlegroups.com; Dailei; linux-arm-ker...@lists.infradead.org; 
aryabi...@virtuozzo.com; labb...@redhat.com; vladimir.mur...@arm.com; 
keesc...@chromium.org; a...@arndb.de; Zengweilin; open...@gmail.com; 
Heshaoliang; t...@linutronix.de; dvyu...@google.com; ard.biesheu...@linaro.org; 
linux-kernel@vger.kernel.org; Jiazhenghua; a...@linux-foundation.org; 
robin.mur...@arm.com; thgar...@google.com; kirill.shute...@linux.intel.com
主题: Re: [PATCH 01/11] Initialize the mapping of KASan shadow memory

On 22/11/17 12:56, Liuwenliang (Abbott Liu) wrote:
> On Nov 22, 2017  20:30  Mark Rutland [mailto:mark.rutl...@arm.com] wrote:
>> On Tue, Nov 21, 2017 at 07:59:01AM +, Liuwenliang (Abbott Liu) wrote:
>>> On Nov 17, 2017  21:49  Marc Zyngier [mailto:marc.zyng...@arm.com]  wrote:
>>>> On Sat, 18 Nov 2017 10:40:08 +
>>>> "Liuwenliang (Abbott Liu)"  wrote:
>>>>> On Nov 17, 2017  15:36 Christoffer Dall [mailto:cd...@linaro.org]  wrote:
> 
>>> Please don't ask people to limit to 4GB of physical space on CPU
>>> supporting LPAE, please don't ask people to guaranteed to have some
>>> memory below 4GB on CPU supporting LPAE.
> 
>> I don't think that Marc is suggesting that you'd always use the 32-bit
>> accessors on an LPAE system, just that all the definitions should exist
>> regardless of configuration.
> 
>> So rather than this:
> 
>>> +#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
> 
>> ... you'd have the following in cp15.h:
> 
>> #define TTBR0_64   __ACCESS_CP15_64(0, c2)
>> #

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

[Liuwenliang (Abbott Liu)]

On Nov 22, 2017  20:30  Mark Rutland [mailto:mark.rutl...@arm.com] wrote:
>On Tue, Nov 21, 2017 at 07:59:01AM +0000, Liuwenliang (Abbott Liu) wrote:
>> On Nov 17, 2017  21:49  Marc Zyngier [mailto:marc.zyng...@arm.com]  wrote:
>> >On Sat, 18 Nov 2017 10:40:08 +0000
>> >"Liuwenliang (Abbott Liu)" <liuwenli...@huawei.com> wrote:
>> >> On Nov 17, 2017  15:36 Christoffer Dall [mailto:cd...@linaro.org]  wrote:

>> Please don't ask people to limit to 4GB of physical space on CPU
>> supporting LPAE, please don't ask people to guaranteed to have some
>> memory below 4GB on CPU supporting LPAE.

>I don't think that Marc is suggesting that you'd always use the 32-bit
>accessors on an LPAE system, just that all the definitions should exist
>regardless of configuration.

>So rather than this:

>> +#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

>... you'd have the following in cp15.h:

>#define TTBR0_64   __ACCESS_CP15_64(0, c2)
>#define TTBR1_64   __ACCESS_CP15_64(1, c2)
>#define PAR_64 __ACCESS_CP15_64(0, c7)

>#define TTBR0_32   __ACCESS_CP15(c2, 0, c0, 0)
>#define TTBR1_32   __ACCESS_CP15(c2, 0, c0, 1)
>#define PAR_32 __ACCESS_CP15(c7, 0, c4, 0)

>... and elsewhere, where it matters, we choose which to use depending on
>the kernel configuration, e.g.

>void set_ttbr0(u64 val)
>{
>   if (IS_ENABLED(CONFIG_ARM_LPAE))
>   write_sysreg(val, TTBR0_64);
>   else
>   write_sysreg(val, TTBR0_32);
>}

>Thanks,
>Mark.

Thanks for your solution.
I didn't know there was a IS_ENABLED macro that I can use, so I can't write a 
function 
like:
void set_ttbr0(u64 val)
{
   if (IS_ENABLED(CONFIG_ARM_LPAE))
   write_sysreg(val, TTBR0_64);
   else
   write_sysreg(val, TTBR0_32);
}


Here is the code I tested on vexpress_a9 and vexpress_a15:
diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
index dbdbce1..5eb0185 100644
--- a/arch/arm/include/asm/cp15.h
+++ b/arch/arm/include/asm/cp15.h
@@ -2,6 +2,7 @@
 #define __ASM_ARM_CP15_H

 #include 
+#include 

 /*
  * CR1 bits (CP#15 CR1)
@@ -64,8 +65,93 @@
 #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_32   __ACCESS_CP15(c2, 0, c0, 0)
+#define TTBR1_32   __ACCESS_CP15(c2, 0, c0, 1)
+#define TTBR0_64   __ACCESS_CP15_64(0, c2)
+#define TTBR1_64   __ACCESS_CP15_64(1, c2)
+#define PAR __ACCESS_CP15_64(0, c7)
+#define VTTBR   __ACCESS_CP15_64(6, c2)
+#define CNTV_CVAL   __ACCESS_CP15_64(3, c14)
+#define CNTVOFF __ACCESS_CP15_64(4, c14)
+
+#define MIDR__ACCESS_CP15(c0, 0, c0, 0)
+#define CSSELR  __ACCESS_CP15(c0, 2, c0, 0)
+#define VPIDR   __ACCESS_CP15(c0, 4, c0, 0)
+#define VMPIDR  __ACCESS_CP15(c0, 4, c0, 5)
+#define SCTLR   __ACCESS_CP15(c1, 0, c0, 0)
+#define CPACR   __ACCESS_CP15(c1, 0, c0, 2)
+#define HCR __ACCESS_CP15(c1, 4, c1, 0)
+#define HDCR__ACCESS_CP15(c1, 4, c1, 1)
+#define HCPTR   __ACCESS_CP15(c1, 4, c1, 2)
+#define HSTR__ACCESS_CP15(c1, 4, c1, 3)
+#define TTBCR   __ACCESS_CP15(c2, 0, c0, 2)
+#define HTCR__ACCESS_CP15(c2, 4, c0, 2)
+#define VTCR__ACCESS_CP15(c2, 4, c1, 2)
+#define DACR__ACCESS_CP15(c3, 0, c0, 0)
+#define DFSR__ACCESS_CP15(c5, 0, c0, 0)
+#define IFSR__ACCESS_CP15(c5, 0, c0, 1)
+#define ADFSR   __ACCESS_CP15(c5, 0, c1, 0)
+#define AIFSR   __ACCESS_CP15(c5, 0, c1, 1)
+#define HSR __ACCESS_CP15(c5, 4, c2, 0)
+#define DFAR__ACCESS_CP15(c6, 0, c0, 0)
+#define IFAR__ACCESS_CP15(c6, 0, c0, 2)
+#define HDFAR   __ACCESS_CP15(c6, 4, c0, 0)
+#define HIFAR   __ACCESS_CP15(c6, 4, c0, 2)
+#define HPFAR   __ACCESS_CP15(c6, 4, c0, 4)
+#define ICIALLUIS   __ACCESS_CP15(c7, 0, c1, 0)
+#define ATS1CPR __ACCESS_CP15(c7, 0, c8, 0)
+#define TLBIALLIS   __ACCESS_CP15(c8, 0, c3, 0)
+#define TLBIALL __ACCESS_CP15(c8, 0, c7, 0)
+#define TLBIALLNSNHIS   __ACCESS_CP15(c8, 4, c3, 4)
+#define PRRR__ACCESS_CP15(c10, 0, c2, 0)
+#define NMRR__ACCESS_CP15(c10, 0, c2, 1)
+#define AMAIR0  __ACCESS_CP15(c10, 0, c3, 0)
+#define AMAIR1  __ACCESS_CP15(c10, 0, c3, 1)
+#define VBAR   

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

[Liuwenliang (Abbott Liu)]

On Nov 22, 2017  20:30  Mark Rutland [mailto:mark.rutl...@arm.com] wrote:
>On Tue, Nov 21, 2017 at 07:59:01AM +0000, Liuwenliang (Abbott Liu) wrote:
>> On Nov 17, 2017  21:49  Marc Zyngier [mailto:marc.zyng...@arm.com]  wrote:
>> >On Sat, 18 Nov 2017 10:40:08 +0000
>> >"Liuwenliang (Abbott Liu)"  wrote:
>> >> On Nov 17, 2017  15:36 Christoffer Dall [mailto:cd...@linaro.org]  wrote:

>> Please don't ask people to limit to 4GB of physical space on CPU
>> supporting LPAE, please don't ask people to guaranteed to have some
>> memory below 4GB on CPU supporting LPAE.

>I don't think that Marc is suggesting that you'd always use the 32-bit
>accessors on an LPAE system, just that all the definitions should exist
>regardless of configuration.

>So rather than this:

>> +#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

>... you'd have the following in cp15.h:

>#define TTBR0_64   __ACCESS_CP15_64(0, c2)
>#define TTBR1_64   __ACCESS_CP15_64(1, c2)
>#define PAR_64 __ACCESS_CP15_64(0, c7)

>#define TTBR0_32   __ACCESS_CP15(c2, 0, c0, 0)
>#define TTBR1_32   __ACCESS_CP15(c2, 0, c0, 1)
>#define PAR_32 __ACCESS_CP15(c7, 0, c4, 0)

>... and elsewhere, where it matters, we choose which to use depending on
>the kernel configuration, e.g.

>void set_ttbr0(u64 val)
>{
>   if (IS_ENABLED(CONFIG_ARM_LPAE))
>   write_sysreg(val, TTBR0_64);
>   else
>   write_sysreg(val, TTBR0_32);
>}

>Thanks,
>Mark.

Thanks for your solution.
I didn't know there was a IS_ENABLED macro that I can use, so I can't write a 
function 
like:
void set_ttbr0(u64 val)
{
   if (IS_ENABLED(CONFIG_ARM_LPAE))
   write_sysreg(val, TTBR0_64);
   else
   write_sysreg(val, TTBR0_32);
}


Here is the code I tested on vexpress_a9 and vexpress_a15:
diff --git a/arch/arm/include/asm/cp15.h b/arch/arm/include/asm/cp15.h
index dbdbce1..5eb0185 100644
--- a/arch/arm/include/asm/cp15.h
+++ b/arch/arm/include/asm/cp15.h
@@ -2,6 +2,7 @@
 #define __ASM_ARM_CP15_H

 #include 
+#include 

 /*
  * CR1 bits (CP#15 CR1)
@@ -64,8 +65,93 @@
 #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_32   __ACCESS_CP15(c2, 0, c0, 0)
+#define TTBR1_32   __ACCESS_CP15(c2, 0, c0, 1)
+#define TTBR0_64   __ACCESS_CP15_64(0, c2)
+#define TTBR1_64   __ACCESS_CP15_64(1, c2)
+#define PAR __ACCESS_CP15_64(0, c7)
+#define VTTBR   __ACCESS_CP15_64(6, c2)
+#define CNTV_CVAL   __ACCESS_CP15_64(3, c14)
+#define CNTVOFF __ACCESS_CP15_64(4, c14)
+
+#define MIDR__ACCESS_CP15(c0, 0, c0, 0)
+#define CSSELR  __ACCESS_CP15(c0, 2, c0, 0)
+#define VPIDR   __ACCESS_CP15(c0, 4, c0, 0)
+#define VMPIDR  __ACCESS_CP15(c0, 4, c0, 5)
+#define SCTLR   __ACCESS_CP15(c1, 0, c0, 0)
+#define CPACR   __ACCESS_CP15(c1, 0, c0, 2)
+#define HCR __ACCESS_CP15(c1, 4, c1, 0)
+#define HDCR__ACCESS_CP15(c1, 4, c1, 1)
+#define HCPTR   __ACCESS_CP15(c1, 4, c1, 2)
+#define HSTR__ACCESS_CP15(c1, 4, c1, 3)
+#define TTBCR   __ACCESS_CP15(c2, 0, c0, 2)
+#define HTCR__ACCESS_CP15(c2, 4, c0, 2)
+#define VTCR__ACCESS_CP15(c2, 4, c1, 2)
+#define DACR__ACCESS_CP15(c3, 0, c0, 0)
+#define DFSR__ACCESS_CP15(c5, 0, c0, 0)
+#define IFSR__ACCESS_CP15(c5, 0, c0, 1)
+#define ADFSR   __ACCESS_CP15(c5, 0, c1, 0)
+#define AIFSR   __ACCESS_CP15(c5, 0, c1, 1)
+#define HSR __ACCESS_CP15(c5, 4, c2, 0)
+#define DFAR__ACCESS_CP15(c6, 0, c0, 0)
+#define IFAR__ACCESS_CP15(c6, 0, c0, 2)
+#define HDFAR   __ACCESS_CP15(c6, 4, c0, 0)
+#define HIFAR   __ACCESS_CP15(c6, 4, c0, 2)
+#define HPFAR   __ACCESS_CP15(c6, 4, c0, 4)
+#define ICIALLUIS   __ACCESS_CP15(c7, 0, c1, 0)
+#define ATS1CPR __ACCESS_CP15(c7, 0, c8, 0)
+#define TLBIALLIS   __ACCESS_CP15(c8, 0, c3, 0)
+#define TLBIALL __ACCESS_CP15(c8, 0, c7, 0)
+#define TLBIALLNSNHIS   __ACCESS_CP15(c8, 4, c3, 4)
+#define PRRR__ACCESS_CP15(c10, 0, c2, 0)
+#define NMRR__ACCESS_CP15(c10, 0, c2, 1)
+#define AMAIR0  __ACCESS_CP15(c10, 0, c3, 0)
+#define AMAIR1  __ACCESS_CP15(c10, 0, c3, 1)
+#define VBAR__ACCESS_CP15(c12, 0, c0, 

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

[Liuwenliang (Abbott Liu)]

On Nov 17, 2017  15:36 Christoffer Dall [mailto:cd...@linaro.org]  wrote:
>If your processor does support LPAE (like a Cortex-A15 for example),
>then you have both the 32-bit accessors (MRC and MCR) and the 64-bit
>accessors (MRRC, MCRR), and using the 32-bit accessor will simply access
>the lower 32-bits of the 64-bit register.
>
>Hope this helps,
>-Christoffer

If you know the higher 32-bits of the 64-bits cp15's register is not useful for 
your system,
then you can use the 32-bit accessor to get or set the 64-bit cp15's register.
But if the higher 32-bits of the 64-bits cp15's register is useful for your 
system,
then you can't use the 32-bit accessor to get or set the 64-bit cp15's register.

TTBR0/TTBR1/PAR's higher 32-bits is useful for CPU supporting LPAE.
The following description which comes from ARM(r) Architecture Reference
Manual ARMv7-A and ARMv7-R edition tell us the reason:

64-bit TTBR0 and TTBR1 format:
...
BADDR, bits[39:x] : 
Translation table base address, bits[39:x]. Defining the translation table base 
address width on
page B4-1698 describes how x is defined.
The value of x determines the required alignment of the translation table, 
which must be aligned to
2x bytes.

Abbott Liu: Because BADDR on CPU supporting LPAE may be bigger than max value 
of 32-bit, so bits[39:32] may 
be valid value which is useful for the system.

64-bit PAR format
...
PA[39:12]
Physical Address. The physical address corresponding to the supplied virtual 
address. This field
returns address bits[39:12].

Abbott Liu: Because Physical Address on CPU supporting LPAE may be bigger than 
max value of 32-bit, 
so bits[39:32] may be valid value which is useful for the system.

Conclusion: Don't use 32-bit accessor to get or set TTBR0/TTBR1/PAR on CPU 
supporting LPAE,
if you do that, your system may run error.

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

[Liuwenliang (Abbott Liu)]

On Nov 17, 2017  15:36 Christoffer Dall [mailto:cd...@linaro.org]  wrote:
>If your processor does support LPAE (like a Cortex-A15 for example),
>then you have both the 32-bit accessors (MRC and MCR) and the 64-bit
>accessors (MRRC, MCRR), and using the 32-bit accessor will simply access
>the lower 32-bits of the 64-bit register.
>
>Hope this helps,
>-Christoffer

If you know the higher 32-bits of the 64-bits cp15's register is not useful for 
your system,
then you can use the 32-bit accessor to get or set the 64-bit cp15's register.
But if the higher 32-bits of the 64-bits cp15's register is useful for your 
system,
then you can't use the 32-bit accessor to get or set the 64-bit cp15's register.

TTBR0/TTBR1/PAR's higher 32-bits is useful for CPU supporting LPAE.
The following description which comes from ARM(r) Architecture Reference
Manual ARMv7-A and ARMv7-R edition tell us the reason:

64-bit TTBR0 and TTBR1 format:
...
BADDR, bits[39:x] : 
Translation table base address, bits[39:x]. Defining the translation table base 
address width on
page B4-1698 describes how x is defined.
The value of x determines the required alignment of the translation table, 
which must be aligned to
2x bytes.

Abbott Liu: Because BADDR on CPU supporting LPAE may be bigger than max value 
of 32-bit, so bits[39:32] may 
be valid value which is useful for the system.

64-bit PAR format
...
PA[39:12]
Physical Address. The physical address corresponding to the supplied virtual 
address. This field
returns address bits[39:12].

Abbott Liu: Because Physical Address on CPU supporting LPAE may be bigger than 
max value of 32-bit, 
so bits[39:32] may be valid value which is useful for the system.

Conclusion: Don't use 32-bit accessor to get or set TTBR0/TTBR1/PAR on CPU 
supporting LPAE,
if you do that, your system may run error.

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

[Liuwenliang (Abbott Liu)]

On 16/11/17  22:41 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>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.

I am sorry. My test is fault. I only defined TTBR0 as __ACCESS_CP15_64,
but I don't use the definition TTBR0 as __ACCESS_CP15_64. 

Now I use the definition TTBR0 as __ACCESS_CP15_64 on CPU supporting
LPAE(vexpress_a9), I find it doesn't work and report undefined instruction error
when execute "mrrc" instruction.

So, you are right that 64bit accessor of TTBR0 cannot work on LPAE.

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

[Liuwenliang (Abbott Liu)]

On 16/11/17  22:41 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>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.

I am sorry. My test is fault. I only defined TTBR0 as __ACCESS_CP15_64,
but I don't use the definition TTBR0 as __ACCESS_CP15_64. 

Now I use the definition TTBR0 as __ACCESS_CP15_64 on CPU supporting
LPAE(vexpress_a9), I find it doesn't work and report undefined instruction error
when execute "mrrc" instruction.

So, you are right that 64bit accessor of TTBR0 cannot work on LPAE.

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

[Liuwenliang (Abbott Liu)]

On 16/11/17  22:41 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>- If the CPU supports LPAE, then both 32 and 64bit accessors work


I don't how 32bit accessor can work on CPU supporting LPAE, give me your 
solution.

Thanks.

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

[Liuwenliang (Abbott Liu)]

On 16/11/17  22:41 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>- If the CPU supports LPAE, then both 32 and 64bit accessors work


I don't how 32bit accessor can work on CPU supporting LPAE, give me your 
solution.

Thanks.

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

[Liuwenliang (Abbott Liu)]

On 16/11/17  17:54 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>On Thu, Nov 16 2017 at  3:07:54 am GMT, "Liuwenliang (Abbott Liu)" 
><liuwenli...@huawei.com> wrote:
>>>On 15/11/17 13:16, Liuwenliang (Abbott Liu) wrote:
>>>> On 09/11/17  18:36 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>>>>> On Wed, Nov 15 2017 at 10:20:02 am GMT, "Liuwenliang (Abbott Liu)"
>>>>> <liuwenli...@huawei.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 c

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

[Liuwenliang (Abbott Liu)]

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

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

[Liuwenliang (Abbott Liu)]

>On 15/11/17 13:16, Liuwenliang (Abbott Liu) wrote:
>> On 09/11/17  18:36 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>>> On Wed, Nov 15 2017 at 10:20:02 am GMT, "Liuwenliang (Abbott Liu)" 
>>> <liuwenli...@huawei.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". 

64-bit TTBR0 and TTBR1 format
The bit assignments for the 64-bit implementations of TTBR0 and TTBR1 are 
identical, and are:
Bits[63:56] UNK/SBZP.
ASID, bits[55:48]:
  An ASID for the translation table base address. The TTBCR.A1 field selects 
either TTBR0.ASID
or TTBR1.ASID.
Bits[47:40] UNK/SBZP.
BADDR, bits[39:x]:
   Translation table base address, bits[39:x]. Defining the translation table 
base address width on
page B4-1698 describes how x is defined.
The value of x determines the required alignment of the translation table, 
which must be aligned to
2x bytes.

Bits[x-1:0] UNK/SBZP.
...
To access a 64-bit TTBR0, software performs a 64-bit read or write of the CP15 
registers with  set to c2 and
 set to 0. For example:
MRRC p15,0,,, c2 ; Read 64-bit TTBR0 into Rt (low word) and Rt2 (high 
word)
MCRR p15,0,,, c2 ; Write Rt (low word) and Rt2 (high word) to 64-bit 
TTBR0

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.

>Also, if I follow your reasoning, why are you bothering defining PAR in
>the non-LPAE case? It is not used by anything, as far as I can see...

I don't use the PAR, I change the defining PAR just because I think it will be 
wrong in
a non LPAE CPU.

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

[Liuwenliang (Abbott Liu)]

>On 15/11/17 13:16, Liuwenliang (Abbott Liu) wrote:
>> On 09/11/17  18:36 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>>> On Wed, Nov 15 2017 at 10:20:02 am GMT, "Liuwenliang (Abbott Liu)" 
>>>  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". 

64-bit TTBR0 and TTBR1 format
The bit assignments for the 64-bit implementations of TTBR0 and TTBR1 are 
identical, and are:
Bits[63:56] UNK/SBZP.
ASID, bits[55:48]:
  An ASID for the translation table base address. The TTBCR.A1 field selects 
either TTBR0.ASID
or TTBR1.ASID.
Bits[47:40] UNK/SBZP.
BADDR, bits[39:x]:
   Translation table base address, bits[39:x]. Defining the translation table 
base address width on
page B4-1698 describes how x is defined.
The value of x determines the required alignment of the translation table, 
which must be aligned to
2x bytes.

Bits[x-1:0] UNK/SBZP.
...
To access a 64-bit TTBR0, software performs a 64-bit read or write of the CP15 
registers with  set to c2 and
 set to 0. For example:
MRRC p15,0,,, c2 ; Read 64-bit TTBR0 into Rt (low word) and Rt2 (high 
word)
MCRR p15,0,,, c2 ; Write Rt (low word) and Rt2 (high word) to 64-bit 
TTBR0

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.

>Also, if I follow your reasoning, why are you bothering defining PAR in
>the non-LPAE case? It is not used by anything, as far as I can see...

I don't use the PAR, I change the defining PAR just because I think it will be 
wrong in
a non LPAE CPU.

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

[Liuwenliang (Abbott Liu)]

On 09/11/17  18:36 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>On Wed, Nov 15 2017 at 10:20:02 am GMT, "Liuwenliang (Abbott Liu)" 
><liuwenli...@huawei.com> wrote:
>> On 09/11/17 18:11, Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>>>On 09/11/17 07:46, Liuwenliang (Abbott Liu) wrote:
>>>> diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
>>>> index 049ee0a..359a782 100644
>>>> --- a/arch/arm/mm/kasan_init.c
>>>> +++ b/arch/arm/mm/kasan_init.c
>>>> @@ -15,6 +15,7 @@
>>>>  #include 
>>>>  #include 
>>>>  #include 
>>>> +#include 
>>>
>>>No, please don't do that. You shouldn't have to include KVM stuff in
>>>unrelated code. Instead of adding stuff to kvm_hyp.h, move all the
>>>__ACCESS_CP15* to cp15.h, and it will be obvious to everyone that this
>>>is where new definition should be added.
>>
>> Thanks for your review.  You are right. It is better to move
>> __ACCESS_CP15* to cp15.h than to include kvm_hyp.h. But I don't think
>> it is a good idea to move registers definition which is used in
>> virtualization to cp15.h, Because there is no virtualization stuff in
>> cp15.h.
>
>It is not about virtualization at all.
>
>It is about what is a CP15 register and what is not. This file is called
>"cp15.h", not "cp15-except-virtualization-and-maybe-some-others.h". But
>at the end of the day, that's for Russell to decide.

Thanks for your review.
You are right, all __ACCESS_CP15* are cp15 registers. I splited normal cp15 
register
(such as TTBR0/TTBR1/SCTLR and so on) and virtualizaton cp15 registers(such as 
VTTBR/
CNTV_CVAL/HCR) because I didn't think we need use those virtualization cp15 
registers
in non virtualization system.

But now I think all __ACCESS_CP15* move to cp15.h may be a better choise. 

>>
>> Here is the code which I tested on vexpress_a15 and vexpress_a9:
>> 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:

B4.1.155 TTBR0, Translation Table Base Register 0, VMSA
...
The Multiprocessing Extensions change the TTBR0 32-bit register format.
The Large Physical Address Extension extends TTBR0 to a 64-bit register. In an
implementation that includes the Large Physical Address Extension, TTBCR.EAE
determines which TTBR0 format is used:
EAE==0 32-bit format is used. TTBR0[63:32] are ignored.
EAE==1 64-bit format is used.

B4.1.156 TTBR1, Translation Table Base Register 1, VMSA
...
The Multiprocessing Extensions change the TTBR0 32-bit register format.
The Large Physical Address Extension extends TTBR1 to a 64-bit register. In an
implementation that includes the Large Physical Address Extension, TTBCR.EAE
determines which TTBR1 format is used:
EAE==0 32-bit format is used. TTBR1[63:32] are ignored.
EAE==1 64-bit format is used.

B4.1.154 TTBCR, Translation Table Base Control Register, VMSA
...
EAE, bit[31], if implementation includes the Large Physical Address Extension
Extended Address Enable. The meanings of the possible values of this bit are:
0   Use the 32-bit translation system, with the Short-descriptor translation 
table format. In
this case, the format of the TTBCR is as described in this section.
1   Use the 40-bit translation system, with the Long-descriptor translation 
table format. In
this case, the format of the TTBCR is as described in TTBCR format when using 
the
Long-descriptor translation table format on page B4-1692.

B4.1.112 PAR, Physical Address Register, VMSA

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

[Liuwenliang (Abbott Liu)]

On 09/11/17  18:36 Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>On Wed, Nov 15 2017 at 10:20:02 am GMT, "Liuwenliang (Abbott Liu)" 
> wrote:
>> On 09/11/17 18:11, Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>>>On 09/11/17 07:46, Liuwenliang (Abbott Liu) wrote:
>>>> diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
>>>> index 049ee0a..359a782 100644
>>>> --- a/arch/arm/mm/kasan_init.c
>>>> +++ b/arch/arm/mm/kasan_init.c
>>>> @@ -15,6 +15,7 @@
>>>>  #include 
>>>>  #include 
>>>>  #include 
>>>> +#include 
>>>
>>>No, please don't do that. You shouldn't have to include KVM stuff in
>>>unrelated code. Instead of adding stuff to kvm_hyp.h, move all the
>>>__ACCESS_CP15* to cp15.h, and it will be obvious to everyone that this
>>>is where new definition should be added.
>>
>> Thanks for your review.  You are right. It is better to move
>> __ACCESS_CP15* to cp15.h than to include kvm_hyp.h. But I don't think
>> it is a good idea to move registers definition which is used in
>> virtualization to cp15.h, Because there is no virtualization stuff in
>> cp15.h.
>
>It is not about virtualization at all.
>
>It is about what is a CP15 register and what is not. This file is called
>"cp15.h", not "cp15-except-virtualization-and-maybe-some-others.h". But
>at the end of the day, that's for Russell to decide.

Thanks for your review.
You are right, all __ACCESS_CP15* are cp15 registers. I splited normal cp15 
register
(such as TTBR0/TTBR1/SCTLR and so on) and virtualizaton cp15 registers(such as 
VTTBR/
CNTV_CVAL/HCR) because I didn't think we need use those virtualization cp15 
registers
in non virtualization system.

But now I think all __ACCESS_CP15* move to cp15.h may be a better choise. 

>>
>> Here is the code which I tested on vexpress_a15 and vexpress_a9:
>> 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:

B4.1.155 TTBR0, Translation Table Base Register 0, VMSA
...
The Multiprocessing Extensions change the TTBR0 32-bit register format.
The Large Physical Address Extension extends TTBR0 to a 64-bit register. In an
implementation that includes the Large Physical Address Extension, TTBCR.EAE
determines which TTBR0 format is used:
EAE==0 32-bit format is used. TTBR0[63:32] are ignored.
EAE==1 64-bit format is used.

B4.1.156 TTBR1, Translation Table Base Register 1, VMSA
...
The Multiprocessing Extensions change the TTBR0 32-bit register format.
The Large Physical Address Extension extends TTBR1 to a 64-bit register. In an
implementation that includes the Large Physical Address Extension, TTBCR.EAE
determines which TTBR1 format is used:
EAE==0 32-bit format is used. TTBR1[63:32] are ignored.
EAE==1 64-bit format is used.

B4.1.154 TTBCR, Translation Table Base Control Register, VMSA
...
EAE, bit[31], if implementation includes the Large Physical Address Extension
Extended Address Enable. The meanings of the possible values of this bit are:
0   Use the 32-bit translation system, with the Short-descriptor translation 
table format. In
this case, the format of the TTBCR is as described in this section.
1   Use the 40-bit translation system, with the Long-descriptor translation 
table format. In
this case, the format of the TTBCR is as described in TTBCR format when using 
the
Long-descriptor translation table format on page B4-1692.

B4.1.112 PAR, Physical Address Register, VMSA
If the implementation includes the Lar

Re: [PATCH 05/11] Disable kasan's instrumentation

[Liuwenliang (Abbott Liu)]

On 19/10/17 20:47, Russell King - ARM Linux [mailto:li...@armlinux.org.uk]  
wrote:
>On Wed, Oct 11, 2017 at 04:22:21PM +0800, Abbott Liu wrote:
>> From: Andrey Ryabinin 
>> 
>>  To avoid some build and runtime errors, compiler's instrumentation must
>>  be disabled for code not linked with kernel image.
>
>How does that explain the change to unwind.c ?

Thanks for your review.
Here is patch code:
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -249,7 +249,8 @@ static int unwind_pop_register(struct unwind_ctrl_block 
*ctrl,
if (*vsp >= (unsigned long *)ctrl->sp_high)
return -URC_FAILURE;

-   ctrl->vrs[reg] = *(*vsp)++;
+   ctrl->vrs[reg] = READ_ONCE_NOCHECK(*(*vsp));
+   (*vsp)++;
return URC_OK;
 }

I change here because I don't think unwind_frame need to be check by kasan, and 
I have ever 
found the following error which rarely appares when remove the change of 
unwind.c.

Here is the error log:
==
BUG: KASAN: stack-out-of-bounds in unwind_frame+0x3e0/0x788
Read of size 4 at addr 868a3b20 by task swapper/0/1

CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc2+ #2
Hardware name: ARM-Versatile Express
[<8011479c>] (unwind_backtrace) from [<8010f558>] (show_stack+0x10/0x14)
[<8010f558>] (show_stack) from [<808fdca0>] (dump_stack+0x90/0xa4)
[<808fdca0>] (dump_stack) from [<802b3808>] 
(print_address_description+0x4c/0x270)
[<802b3808>] (print_address_description) from [<802b3ec4>] 
(kasan_report+0x218/0x300)
[<802b3ec4>] (kasan_report) from [<801143f4>] (unwind_frame+0x3e0/0x788)
[<801143f4>] (unwind_frame) from [<8010ebc4>] (walk_stackframe+0x2c/0x38)
[<8010ebc4>] (walk_stackframe) from [<8010ee70>] 
(__save_stack_trace+0x160/0x164)
[<8010ee70>] (__save_stack_trace) from [<802b342c>] (kasan_slab_free+0x84/0x158)
[<802b342c>] (kasan_slab_free) from [<802b05dc>] (kmem_cache_free+0x58/0x1d4)
[<802b05dc>] (kmem_cache_free) from [<801a6420>] 
(rcu_process_callbacks+0x600/0xe04)
[<801a6420>] (rcu_process_callbacks) from [<801018e8>] 
(__do_softirq+0x1a0/0x4e0)
[<801018e8>] (__do_softirq) from [<80131560>] (irq_exit+0xec/0x120)
[<80131560>] (irq_exit) from [<8018d2a0>] (__handle_domain_irq+0x78/0xdc)
[<8018d2a0>] (__handle_domain_irq) from [<80101700>] (gic_handle_irq+0x48/0x8c)
[<80101700>] (gic_handle_irq) from [<80110690>] (__irq_svc+0x70/0x94)
Exception stack(0x868a39f0 to 0x868a3a38)
39e0: 7fff 868a3b88  0001
3a00: 868a3b84 7fff 868a3b88 6fd1474c 868a3ac0 868a 0002 86898000
3a20: 0001 868a3a40 8091b4d4 8091edb0 6013 
[<80110690>] (__irq_svc) from [<8091edb0>] (schedule_timeout+0x0/0x3c4)
[<8091edb0>] (schedule_timeout) from [<6fd14770>] (0x6fd14770)

The buggy address belongs to the page:
page:87fcc460 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x0()
raw:      87fcc474 87fcc474 
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 868a3a00: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
 868a3a80: 00 00 04 f4 f3 f3 f3 f3 00 00 00 00 00 00 00 00
>868a3b00: 00 00 00 00 f1 f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2
   ^
 868a3b80: 00 00 00 00 00 04 f4 f4 f3 f3 f3 f3 00 00 00 00
 868a3c00: 00 00 00 00 f1 f1 f1 f1 00 07 f4 f4 f3 f3 f3 f3
==
Disabling lock debugging due to kernel taint

/* Before poping a register check whether it is feasible or not */
static int unwind_pop_register(struct unwind_ctrl_block *ctrl,
unsigned long **vsp, unsigned int reg)
{
if (unlikely(ctrl->check_each_pop))
if (*vsp >= (unsigned long *)ctrl->sp_high)
return -URC_FAILURE;

// unwind_frame+0x3e0/0x788 is here
ctrl->vrs[reg] = *(*vsp)++;
return URC_OK;
}
>
>Does this also disable the string macro changes?
>
>In any case, this should certainly precede patch 4, and very probably
>patch 2.

You are right. I will change it in net version.

Re: [PATCH 05/11] Disable kasan's instrumentation

[Liuwenliang (Abbott Liu)]

On 19/10/17 20:47, Russell King - ARM Linux [mailto:li...@armlinux.org.uk]  
wrote:
>On Wed, Oct 11, 2017 at 04:22:21PM +0800, Abbott Liu wrote:
>> From: Andrey Ryabinin 
>> 
>>  To avoid some build and runtime errors, compiler's instrumentation must
>>  be disabled for code not linked with kernel image.
>
>How does that explain the change to unwind.c ?

Thanks for your review.
Here is patch code:
--- a/arch/arm/kernel/unwind.c
+++ b/arch/arm/kernel/unwind.c
@@ -249,7 +249,8 @@ static int unwind_pop_register(struct unwind_ctrl_block 
*ctrl,
if (*vsp >= (unsigned long *)ctrl->sp_high)
return -URC_FAILURE;

-   ctrl->vrs[reg] = *(*vsp)++;
+   ctrl->vrs[reg] = READ_ONCE_NOCHECK(*(*vsp));
+   (*vsp)++;
return URC_OK;
 }

I change here because I don't think unwind_frame need to be check by kasan, and 
I have ever 
found the following error which rarely appares when remove the change of 
unwind.c.

Here is the error log:
==
BUG: KASAN: stack-out-of-bounds in unwind_frame+0x3e0/0x788
Read of size 4 at addr 868a3b20 by task swapper/0/1

CPU: 1 PID: 1 Comm: swapper/0 Not tainted 4.13.0-rc2+ #2
Hardware name: ARM-Versatile Express
[<8011479c>] (unwind_backtrace) from [<8010f558>] (show_stack+0x10/0x14)
[<8010f558>] (show_stack) from [<808fdca0>] (dump_stack+0x90/0xa4)
[<808fdca0>] (dump_stack) from [<802b3808>] 
(print_address_description+0x4c/0x270)
[<802b3808>] (print_address_description) from [<802b3ec4>] 
(kasan_report+0x218/0x300)
[<802b3ec4>] (kasan_report) from [<801143f4>] (unwind_frame+0x3e0/0x788)
[<801143f4>] (unwind_frame) from [<8010ebc4>] (walk_stackframe+0x2c/0x38)
[<8010ebc4>] (walk_stackframe) from [<8010ee70>] 
(__save_stack_trace+0x160/0x164)
[<8010ee70>] (__save_stack_trace) from [<802b342c>] (kasan_slab_free+0x84/0x158)
[<802b342c>] (kasan_slab_free) from [<802b05dc>] (kmem_cache_free+0x58/0x1d4)
[<802b05dc>] (kmem_cache_free) from [<801a6420>] 
(rcu_process_callbacks+0x600/0xe04)
[<801a6420>] (rcu_process_callbacks) from [<801018e8>] 
(__do_softirq+0x1a0/0x4e0)
[<801018e8>] (__do_softirq) from [<80131560>] (irq_exit+0xec/0x120)
[<80131560>] (irq_exit) from [<8018d2a0>] (__handle_domain_irq+0x78/0xdc)
[<8018d2a0>] (__handle_domain_irq) from [<80101700>] (gic_handle_irq+0x48/0x8c)
[<80101700>] (gic_handle_irq) from [<80110690>] (__irq_svc+0x70/0x94)
Exception stack(0x868a39f0 to 0x868a3a38)
39e0: 7fff 868a3b88  0001
3a00: 868a3b84 7fff 868a3b88 6fd1474c 868a3ac0 868a 0002 86898000
3a20: 0001 868a3a40 8091b4d4 8091edb0 6013 
[<80110690>] (__irq_svc) from [<8091edb0>] (schedule_timeout+0x0/0x3c4)
[<8091edb0>] (schedule_timeout) from [<6fd14770>] (0x6fd14770)

The buggy address belongs to the page:
page:87fcc460 count:0 mapcount:0 mapping:  (null) index:0x0
flags: 0x0()
raw:      87fcc474 87fcc474 
page dumped because: kasan: bad access detected

Memory state around the buggy address:
 868a3a00: 00 00 00 00 00 00 00 00 00 00 00 00 f1 f1 f1 f1
 868a3a80: 00 00 04 f4 f3 f3 f3 f3 00 00 00 00 00 00 00 00
>868a3b00: 00 00 00 00 f1 f1 f1 f1 04 f4 f4 f4 f2 f2 f2 f2
   ^
 868a3b80: 00 00 00 00 00 04 f4 f4 f3 f3 f3 f3 00 00 00 00
 868a3c00: 00 00 00 00 f1 f1 f1 f1 00 07 f4 f4 f3 f3 f3 f3
==
Disabling lock debugging due to kernel taint

/* Before poping a register check whether it is feasible or not */
static int unwind_pop_register(struct unwind_ctrl_block *ctrl,
unsigned long **vsp, unsigned int reg)
{
if (unlikely(ctrl->check_each_pop))
if (*vsp >= (unsigned long *)ctrl->sp_high)
return -URC_FAILURE;

// unwind_frame+0x3e0/0x788 is here
ctrl->vrs[reg] = *(*vsp)++;
return URC_OK;
}
>
>Does this also disable the string macro changes?
>
>In any case, this should certainly precede patch 4, and very probably
>patch 2.

You are right. I will change it in net version.

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

[Liuwenliang (Abbott Liu)]

On 09/11/17 18:11, Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>On 09/11/17 07:46, Liuwenliang (Abbott Liu) wrote:
>> diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
>> index 049ee0a..359a782 100644
>> --- a/arch/arm/mm/kasan_init.c
>> +++ b/arch/arm/mm/kasan_init.c
>> @@ -15,6 +15,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>
>No, please don't do that. You shouldn't have to include KVM stuff in
>unrelated code. Instead of adding stuff to kvm_hyp.h, move all the
>__ACCESS_CP15* to cp15.h, and it will be obvious to everyone that this
>is where new definition should be added.

Thanks for your review.
You are right. It is better to move __ACCESS_CP15* to cp15.h than to include
kvm_hyp.h. But I don't think it is a good idea to move registers definition 
which
is used in virtualization to cp15.h, Because there is no virtualization stuff in
cp15.h.

Here is the code which I tested on vexpress_a15 and vexpress_a9:
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
+#define MIDR__ACCESS_CP15(c0, 0, c0, 0)
+#define CSSELR  __ACCESS_CP15(c0, 2, c0, 0)
+#define SCTLR   __ACCESS_CP15(c1, 0, c0, 0)
+#define CPACR   __ACCESS_CP15(c1, 0, c0, 2)
+#define TTBCR   __ACCESS_CP15(c2, 0, c0, 2)
+#define DACR__ACCESS_CP15(c3, 0, c0, 0)
+#define DFSR__ACCESS_CP15(c5, 0, c0, 0)
+#define IFSR__ACCESS_CP15(c5, 0, c0, 1)
+#define ADFSR   __ACCESS_CP15(c5, 0, c1, 0)
+#define AIFSR   __ACCESS_CP15(c5, 0, c1, 1)
+#define DFAR__ACCESS_CP15(c6, 0, c0, 0)
+#define IFAR__ACCESS_CP15(c6, 0, c0, 2)
+#define ICIALLUIS   __ACCESS_CP15(c7, 0, c1, 0)
+#define ATS1CPR __ACCESS_CP15(c7, 0, c8, 0)
+#define TLBIALLIS   __ACCESS_CP15(c8, 0, c3, 0)
+#define TLBIALL __ACCESS_CP15(c8, 0, c7, 0)
+#define TLBIALLNSNHIS   __ACCESS_CP15(c8, 4, c3, 4)
+#define PRRR__ACCESS_CP15(c10, 0, c2, 0)
+#define NMRR__ACCESS_CP15(c10, 0, c2, 1)
+#define AMAIR0  __ACCESS_CP15(c10, 0, c3, 0)
+#define AMAIR1  __ACCESS_CP15(c10, 0, c3, 1)
+#define CID __ACCESS_CP15(c13, 0, c0, 1)
+#define TID_URW __ACCESS_CP15(c13, 0, c0, 2)
+#define TID_URO __ACCESS_CP15(c13, 0, c0, 3)
+#define TID_PRIV__ACCESS_CP15(c13, 0, c0, 4)
+#define CNTKCTL __ACCESS_CP15(c14, 0, c1, 0)
+#define CNTHCTL __ACCESS_CP15(c14, 4, c1, 0)
+
 extern unsigned long cr_alignment; /* defined in entry-armv.S */

 static inline unsigned long get_cr(void)
diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
index 14b5903..db8d8db 100644
--- a/arch/arm/include/asm/kvm_hyp.h
+++ b/arch/arm/include/asm/kvm_hyp.h
@@ -37,55 +37,25 @@
__val;  \
 })

-#define TTBR0  __ACCESS_CP15_64(0, c2)
-#define TTBR1  __ACCESS_CP15_64(1, c2)
 #define VTTBR  __ACCESS_CP15_64(6, c2)
-#define PAR__ACCESS_CP15_64(0, c7)
 #define CNTV_CVAL  __ACCESS_CP15_64(3, c14)
 #define CNTVOFF__ACCESS_CP15_64(4, c14)

-#define MIDR   __ACCESS_CP15(c0, 0, c0, 0)
-#define CSSELR __ACCESS_CP15(c0, 2, c0, 0)
 #define VPIDR  __ACCESS_CP15(c0, 4, c0, 0)
 #define VMPIDR __ACCESS_CP15(c0, 4, c0, 5)
-#define SCTLR  __ACCESS_CP15(c1, 0, c0, 0)
-#define CPACR  __ACCESS_CP15(c1, 0, c0, 2)
 #define HCR__ACCESS_CP15(c1, 4, c1, 0)
 #define HDCR   __ACCESS_CP15(c1, 4, c1, 1)
 #define HCPTR  __ACCESS_CP15(c1, 4, c1, 2)
 #define HSTR   __ACCESS_CP15(c1, 4, c1, 3)
-#define TTBCR  __ACCESS_CP15(c2, 0, c0, 2)
 #define HTCR   __ACCESS_CP15(c2, 4, c0, 2)
 #define VTCR   __ACCESS_CP15(c2, 4, c1, 2)
-#define DACR   __ACCESS_CP15(c3, 0, c0, 0)
-#define DFSR   __ACCESS_CP15(c5, 0, c0, 0)
-#define IFSR   __ACCESS_CP15(c5, 0, c0, 1)
-#define ADFSR  __ACCESS_CP15(c5, 0, c1, 0)
-#define AIFSR  __ACCESS_CP15(c5, 0, c1, 1)
 #define HSR__ACCESS_CP15(c5, 4, c2, 0)
-#define DFAR   __ACCESS_CP15(c6, 0, c0, 0)
-#define IFAR   __

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

[Liuwenliang (Abbott Liu)]

On 09/11/17 18:11, Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
>On 09/11/17 07:46, Liuwenliang (Abbott Liu) wrote:
>> diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
>> index 049ee0a..359a782 100644
>> --- a/arch/arm/mm/kasan_init.c
>> +++ b/arch/arm/mm/kasan_init.c
>> @@ -15,6 +15,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>
>No, please don't do that. You shouldn't have to include KVM stuff in
>unrelated code. Instead of adding stuff to kvm_hyp.h, move all the
>__ACCESS_CP15* to cp15.h, and it will be obvious to everyone that this
>is where new definition should be added.

Thanks for your review.
You are right. It is better to move __ACCESS_CP15* to cp15.h than to include
kvm_hyp.h. But I don't think it is a good idea to move registers definition 
which
is used in virtualization to cp15.h, Because there is no virtualization stuff in
cp15.h.

Here is the code which I tested on vexpress_a15 and vexpress_a9:
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
+#define MIDR__ACCESS_CP15(c0, 0, c0, 0)
+#define CSSELR  __ACCESS_CP15(c0, 2, c0, 0)
+#define SCTLR   __ACCESS_CP15(c1, 0, c0, 0)
+#define CPACR   __ACCESS_CP15(c1, 0, c0, 2)
+#define TTBCR   __ACCESS_CP15(c2, 0, c0, 2)
+#define DACR__ACCESS_CP15(c3, 0, c0, 0)
+#define DFSR__ACCESS_CP15(c5, 0, c0, 0)
+#define IFSR__ACCESS_CP15(c5, 0, c0, 1)
+#define ADFSR   __ACCESS_CP15(c5, 0, c1, 0)
+#define AIFSR   __ACCESS_CP15(c5, 0, c1, 1)
+#define DFAR__ACCESS_CP15(c6, 0, c0, 0)
+#define IFAR__ACCESS_CP15(c6, 0, c0, 2)
+#define ICIALLUIS   __ACCESS_CP15(c7, 0, c1, 0)
+#define ATS1CPR __ACCESS_CP15(c7, 0, c8, 0)
+#define TLBIALLIS   __ACCESS_CP15(c8, 0, c3, 0)
+#define TLBIALL __ACCESS_CP15(c8, 0, c7, 0)
+#define TLBIALLNSNHIS   __ACCESS_CP15(c8, 4, c3, 4)
+#define PRRR__ACCESS_CP15(c10, 0, c2, 0)
+#define NMRR__ACCESS_CP15(c10, 0, c2, 1)
+#define AMAIR0  __ACCESS_CP15(c10, 0, c3, 0)
+#define AMAIR1  __ACCESS_CP15(c10, 0, c3, 1)
+#define CID __ACCESS_CP15(c13, 0, c0, 1)
+#define TID_URW __ACCESS_CP15(c13, 0, c0, 2)
+#define TID_URO __ACCESS_CP15(c13, 0, c0, 3)
+#define TID_PRIV__ACCESS_CP15(c13, 0, c0, 4)
+#define CNTKCTL __ACCESS_CP15(c14, 0, c1, 0)
+#define CNTHCTL __ACCESS_CP15(c14, 4, c1, 0)
+
 extern unsigned long cr_alignment; /* defined in entry-armv.S */

 static inline unsigned long get_cr(void)
diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
index 14b5903..db8d8db 100644
--- a/arch/arm/include/asm/kvm_hyp.h
+++ b/arch/arm/include/asm/kvm_hyp.h
@@ -37,55 +37,25 @@
__val;  \
 })

-#define TTBR0  __ACCESS_CP15_64(0, c2)
-#define TTBR1  __ACCESS_CP15_64(1, c2)
 #define VTTBR  __ACCESS_CP15_64(6, c2)
-#define PAR__ACCESS_CP15_64(0, c7)
 #define CNTV_CVAL  __ACCESS_CP15_64(3, c14)
 #define CNTVOFF__ACCESS_CP15_64(4, c14)

-#define MIDR   __ACCESS_CP15(c0, 0, c0, 0)
-#define CSSELR __ACCESS_CP15(c0, 2, c0, 0)
 #define VPIDR  __ACCESS_CP15(c0, 4, c0, 0)
 #define VMPIDR __ACCESS_CP15(c0, 4, c0, 5)
-#define SCTLR  __ACCESS_CP15(c1, 0, c0, 0)
-#define CPACR  __ACCESS_CP15(c1, 0, c0, 2)
 #define HCR__ACCESS_CP15(c1, 4, c1, 0)
 #define HDCR   __ACCESS_CP15(c1, 4, c1, 1)
 #define HCPTR  __ACCESS_CP15(c1, 4, c1, 2)
 #define HSTR   __ACCESS_CP15(c1, 4, c1, 3)
-#define TTBCR  __ACCESS_CP15(c2, 0, c0, 2)
 #define HTCR   __ACCESS_CP15(c2, 4, c0, 2)
 #define VTCR   __ACCESS_CP15(c2, 4, c1, 2)
-#define DACR   __ACCESS_CP15(c3, 0, c0, 0)
-#define DFSR   __ACCESS_CP15(c5, 0, c0, 0)
-#define IFSR   __ACCESS_CP15(c5, 0, c0, 1)
-#define ADFSR  __ACCESS_CP15(c5, 0, c1, 0)
-#define AIFSR  __ACCESS_CP15(c5, 0, c1, 1)
 #define HSR__ACCESS_CP15(c5, 4, c2, 0)
-#define DFAR   __ACCESS_CP15(c6, 0, c0, 0)
-#define IFAR   __

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

[Liuwenliang (Abbott Liu)]

On 12/10/17 15:59, Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
> On 11/10/17 09:22, Abbott Liu wrote:
>> diff --git a/arch/arm/include/asm/proc-fns.h 
>> b/arch/arm/include/asm/proc-fns.h
>> index f2e1af4..6e26714 100644
>> --- a/arch/arm/include/asm/proc-fns.h
>> +++ b/arch/arm/include/asm/proc-fns.h
>> @@ -131,6 +131,15 @@ extern void cpu_resume(void);
>>  pg &= ~(PTRS_PER_PGD*sizeof(pgd_t)-1);  \
>>  (pgd_t *)phys_to_virt(pg);  \
>>  })
>> +
>> +#define cpu_set_ttbr0(val)  \
>> +do {\
>> +u64 ttbr = val; \
>> +__asm__("mcrr   p15, 0, %Q0, %R0, c2"   \
>> +: : "r" (ttbr));\
>> +} while (0)
>> +
>> +
>>  #else
>>  #define cpu_get_pgd()   \
>>  ({  \
>> @@ -140,6 +149,30 @@ extern void cpu_resume(void);
>>  pg &= ~0x3fff;  \
>>  (pgd_t *)phys_to_virt(pg);  \
>>  })
>> +
>> +#define cpu_set_ttbr(nr, val)   \
>> +do {\
>> +u64 ttbr = val; \
>> +__asm__("mcrp15, 0, %0, c2, c0, 0"  \
>> +: : "r" (ttbr));\
>> +} while (0)
>> +
>> +#define cpu_get_ttbr(nr)\
>> +({  \
>> +unsigned long ttbr; \
>> +__asm__("mrcp15, 0, %0, c2, c0, 0"  \
>> +: "=r" (ttbr)); \
>> +ttbr;   \
>> +})
>> +
>> +#define cpu_set_ttbr0(val)  \
>> +do {\
>> +u64 ttbr = val; \
>> +__asm__("mcrp15, 0, %0, c2, c0, 0"  \
>> +: : "r" (ttbr));\
>> +} while (0)
>> +
>> +
>
>You could instead lift and extend the definitions provided in kvm_hyp.h,
>and use the read_sysreg/write_sysreg helpers defined in cp15.h.

Thanks for your review. 
I extend definitions of TTBR0/TTBR1/PAR in kvm_hyp.h when the CONFIG_ARM_LPAE 
is 
not defined. 
Because cortex A9 don't support virtualization, so use CONFIG_ARM_LPAE to 
exclude
some functions and macros which are only used in virtualization.

Here is the code which I tested on vexpress_a15 and vexpress_a9:

diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
index 14b5903..2592608 100644
--- a/arch/arm/include/asm/kvm_hyp.h
+++ b/arch/arm/include/asm/kvm_hyp.h
@@ -19,12 +19,14 @@
 #define __ARM_KVM_HYP_H__

 #include 
-#include 
 #include 
+
+#ifdef CONFIG_ARM_LPAE
+#include 
 #include 
 #include 
-
 #define __hyp_text __section(.hyp.text) notrace
+#endif

 #define __ACCESS_VFP(CRn)  \
"mrc", "mcr", __stringify(p10, 7, %0, CRn, cr0, 0), u32
@@ -37,12 +39,18 @@
__val;  \
 })

+#ifdef CONFIG_ARM_LPAE
 #define TTBR0  __ACCESS_CP15_64(0, c2)
 #define TTBR1  __ACCESS_CP15_64(1, c2)
 #define VTTBR  __ACCESS_CP15_64(6, c2)
 #define PAR__ACCESS_CP15_64(0, c7)
 #define CNTV_CVAL  __ACCESS_CP15_64(3, c14)
 #define CNTVOFF__ACCESS_CP15_64(4, c14)
+#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

 #define MIDR   __ACCESS_CP15(c0, 0, c0, 0)
 #define CSSELR __ACCESS_CP15(c0, 2, c0, 0)
@@ -98,6 +106,7 @@
 #define cntvoff_el2CNTVOFF
 #define cnthctl_el2CNTHCTL

+#ifdef CONFIG_ARM_LPAE
 void __timer_save_state(struct kvm_vcpu *vcpu);
 void __timer_restore_state(struct kvm_vcpu *vcpu);

@@ -123,5 +132,6 @@ void __hyp_text __banked_restore_state(struct 
kvm_cpu_context *ctxt);
 asmlinkage int __guest_enter(struct kvm_vcpu *vcpu,
 struct kvm_cpu_context *host);
 asmlinkage int __hyp_do_panic(const char *, int, u32);
+#endif

 #endif /* __ARM_KVM_HYP_H__ */
diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
index 049ee0a..359a782 100644
--- a/arch/arm/mm/kasan_init.c
+++ b/arch/arm/mm/kasan_init.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 #include "mm.h"
@@ -203,16 +204,16 @@ void __init kasan_init(void)
u64 orig_ttbr0;
int i;

-   orig_ttbr0 = cpu_get_ttbr(0);
+ orig_ttbr0 = read_sysreg(TTBR0);

 #ifdef CONFIG_ARM_LPAE
memcpy(tmp_pmd_table, 
pgd_page_vaddr(*pgd_offset_k(KASAN_SHADOW_START)), 

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

[Liuwenliang (Abbott Liu)]

On 12/10/17 15:59, Marc Zyngier [mailto:marc.zyng...@arm.com] wrote:
> On 11/10/17 09:22, Abbott Liu wrote:
>> diff --git a/arch/arm/include/asm/proc-fns.h 
>> b/arch/arm/include/asm/proc-fns.h
>> index f2e1af4..6e26714 100644
>> --- a/arch/arm/include/asm/proc-fns.h
>> +++ b/arch/arm/include/asm/proc-fns.h
>> @@ -131,6 +131,15 @@ extern void cpu_resume(void);
>>  pg &= ~(PTRS_PER_PGD*sizeof(pgd_t)-1);  \
>>  (pgd_t *)phys_to_virt(pg);  \
>>  })
>> +
>> +#define cpu_set_ttbr0(val)  \
>> +do {\
>> +u64 ttbr = val; \
>> +__asm__("mcrr   p15, 0, %Q0, %R0, c2"   \
>> +: : "r" (ttbr));\
>> +} while (0)
>> +
>> +
>>  #else
>>  #define cpu_get_pgd()   \
>>  ({  \
>> @@ -140,6 +149,30 @@ extern void cpu_resume(void);
>>  pg &= ~0x3fff;  \
>>  (pgd_t *)phys_to_virt(pg);  \
>>  })
>> +
>> +#define cpu_set_ttbr(nr, val)   \
>> +do {\
>> +u64 ttbr = val; \
>> +__asm__("mcrp15, 0, %0, c2, c0, 0"  \
>> +: : "r" (ttbr));\
>> +} while (0)
>> +
>> +#define cpu_get_ttbr(nr)\
>> +({  \
>> +unsigned long ttbr; \
>> +__asm__("mrcp15, 0, %0, c2, c0, 0"  \
>> +: "=r" (ttbr)); \
>> +ttbr;   \
>> +})
>> +
>> +#define cpu_set_ttbr0(val)  \
>> +do {\
>> +u64 ttbr = val; \
>> +__asm__("mcrp15, 0, %0, c2, c0, 0"  \
>> +: : "r" (ttbr));\
>> +} while (0)
>> +
>> +
>
>You could instead lift and extend the definitions provided in kvm_hyp.h,
>and use the read_sysreg/write_sysreg helpers defined in cp15.h.

Thanks for your review. 
I extend definitions of TTBR0/TTBR1/PAR in kvm_hyp.h when the CONFIG_ARM_LPAE 
is 
not defined. 
Because cortex A9 don't support virtualization, so use CONFIG_ARM_LPAE to 
exclude
some functions and macros which are only used in virtualization.

Here is the code which I tested on vexpress_a15 and vexpress_a9:

diff --git a/arch/arm/include/asm/kvm_hyp.h b/arch/arm/include/asm/kvm_hyp.h
index 14b5903..2592608 100644
--- a/arch/arm/include/asm/kvm_hyp.h
+++ b/arch/arm/include/asm/kvm_hyp.h
@@ -19,12 +19,14 @@
 #define __ARM_KVM_HYP_H__

 #include 
-#include 
 #include 
+
+#ifdef CONFIG_ARM_LPAE
+#include 
 #include 
 #include 
-
 #define __hyp_text __section(.hyp.text) notrace
+#endif

 #define __ACCESS_VFP(CRn)  \
"mrc", "mcr", __stringify(p10, 7, %0, CRn, cr0, 0), u32
@@ -37,12 +39,18 @@
__val;  \
 })

+#ifdef CONFIG_ARM_LPAE
 #define TTBR0  __ACCESS_CP15_64(0, c2)
 #define TTBR1  __ACCESS_CP15_64(1, c2)
 #define VTTBR  __ACCESS_CP15_64(6, c2)
 #define PAR__ACCESS_CP15_64(0, c7)
 #define CNTV_CVAL  __ACCESS_CP15_64(3, c14)
 #define CNTVOFF__ACCESS_CP15_64(4, c14)
+#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

 #define MIDR   __ACCESS_CP15(c0, 0, c0, 0)
 #define CSSELR __ACCESS_CP15(c0, 2, c0, 0)
@@ -98,6 +106,7 @@
 #define cntvoff_el2CNTVOFF
 #define cnthctl_el2CNTHCTL

+#ifdef CONFIG_ARM_LPAE
 void __timer_save_state(struct kvm_vcpu *vcpu);
 void __timer_restore_state(struct kvm_vcpu *vcpu);

@@ -123,5 +132,6 @@ void __hyp_text __banked_restore_state(struct 
kvm_cpu_context *ctxt);
 asmlinkage int __guest_enter(struct kvm_vcpu *vcpu,
 struct kvm_cpu_context *host);
 asmlinkage int __hyp_do_panic(const char *, int, u32);
+#endif

 #endif /* __ARM_KVM_HYP_H__ */
diff --git a/arch/arm/mm/kasan_init.c b/arch/arm/mm/kasan_init.c
index 049ee0a..359a782 100644
--- a/arch/arm/mm/kasan_init.c
+++ b/arch/arm/mm/kasan_init.c
@@ -15,6 +15,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 

 #include "mm.h"
@@ -203,16 +204,16 @@ void __init kasan_init(void)
u64 orig_ttbr0;
int i;

-   orig_ttbr0 = cpu_get_ttbr(0);
+ orig_ttbr0 = read_sysreg(TTBR0);

 #ifdef CONFIG_ARM_LPAE
memcpy(tmp_pmd_table, 
pgd_page_vaddr(*pgd_offset_k(KASAN_SHADOW_START)),