[PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[Pavel Fedin]

After vGIC initialization succeeded, and timer initialization failed,
the following crash can be observed on ARM32:

kvm [1]: interrupt-controller@10484000 IRQ57
kvm [1]: kvm_arch_timer: can't find DT node
Unable to handle kernel paging request at virtual address 90484000
pgd = c0003000
[90484000] *pgd=80000040006003, *pmd=00000000
Internal error: Oops: 2a06 [#1] PREEMPT SMP ARM
...
[<c003b790>] (v7_flush_kern_dcache_area) from [<c001d638>] (kvm_flush_dcache_pte+0x48/0x5c)
[<c001d638>] (kvm_flush_dcache_pte) from [<c001d898>] (unmap_range+0x24c/0x460)
[<c001d898>] (unmap_range) from [<c001ea4c>] (free_hyp_pgds+0x84/0x160)
[<c001ea4c>] (free_hyp_pgds) from [<c001c85c>] (kvm_arch_init+0x254/0x41c)
[<c001c85c>] (kvm_arch_init) from [<c00122b0>] (kvm_init+0x28/0x2b4)
[<c00122b0>] (kvm_init) from [<c0009988>] (do_one_initcall+0x9c/0x200)

This happens when unmapping reaches mapped vGIC control registers. The
problem root seems to be combination of two facts:
1. vGIC control region is defined in device trees as having size of
   0x2000. But the specification defines only registers up to 0x1FC,
   therefore it is only one page, not two.
2. unmap_ptes() is expected to recognize device memory and omit cache
   flushing. However, it tests only for PAGE_S2_DEVICE, while devices
   mapped for HYP mode have PAGE_HYP_DEVICE, which is different.
   Therefore, cache flush is attempted, and it dies when hitting the
   nonexistent second page.

This patch fixes the problem by adding missing recognition of
PAGE_HYP_DEVICE protection value.

The crash can be observed on Exynos 5410 (and probably on all Exynos5
family) with stock device trees (using MCT) and CONFIG_KVM enabled.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
---
 arch/arm/kvm/mmu.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 7b42012..839dd970 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -213,7 +213,10 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
 			kvm_tlb_flush_vmid_ipa(kvm, addr);
 
 			/* No need to invalidate the cache for device mappings */
-			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
+			if (((pte_val(old_pte) & PAGE_S2_DEVICE)
+			     != PAGE_S2_DEVICE) &&
+			    ((pte_val(old_pte) & PAGE_HYP_DEVICE)
+			     != PAGE_HYP_DEVICE))
 				kvm_flush_dcache_pte(old_pte);
 
 			put_page(virt_to_page(pte));
-- 
2.4.4

Re: [PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[Christoffer Dall]

Hi Pavel,

On Tue, Oct 27, 2015 at 10:40:08AM +0300, Pavel Fedin wrote:
> After vGIC initialization succeeded, and timer initialization failed,
> the following crash can be observed on ARM32:
> 
> kvm [1]: interrupt-controller@10484000 IRQ57
> kvm [1]: kvm_arch_timer: can't find DT node
> Unable to handle kernel paging request at virtual address 90484000
> pgd = c0003000
> [90484000] *pgd=80000040006003, *pmd=00000000
> Internal error: Oops: 2a06 [#1] PREEMPT SMP ARM
> ...
> [<c003b790>] (v7_flush_kern_dcache_area) from [<c001d638>] (kvm_flush_dcache_pte+0x48/0x5c)
> [<c001d638>] (kvm_flush_dcache_pte) from [<c001d898>] (unmap_range+0x24c/0x460)
> [<c001d898>] (unmap_range) from [<c001ea4c>] (free_hyp_pgds+0x84/0x160)
> [<c001ea4c>] (free_hyp_pgds) from [<c001c85c>] (kvm_arch_init+0x254/0x41c)
> [<c001c85c>] (kvm_arch_init) from [<c00122b0>] (kvm_init+0x28/0x2b4)
> [<c00122b0>] (kvm_init) from [<c0009988>] (do_one_initcall+0x9c/0x200)
> 
> This happens when unmapping reaches mapped vGIC control registers. The
> problem root seems to be combination of two facts:
> 1. vGIC control region is defined in device trees as having size of
>    0x2000. But the specification defines only registers up to 0x1FC,
>    therefore it is only one page, not two.
> 2. unmap_ptes() is expected to recognize device memory and omit cache
>    flushing. However, it tests only for PAGE_S2_DEVICE, while devices
>    mapped for HYP mode have PAGE_HYP_DEVICE, which is different.
>    Therefore, cache flush is attempted, and it dies when hitting the
>    nonexistent second page.
> 
> This patch fixes the problem by adding missing recognition of
> PAGE_HYP_DEVICE protection value.
> 
> The crash can be observed on Exynos 5410 (and probably on all Exynos5
> family) with stock device trees (using MCT) and CONFIG_KVM enabled.
> 
> Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
> ---
>  arch/arm/kvm/mmu.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 7b42012..839dd970 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -213,7 +213,10 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
>  			kvm_tlb_flush_vmid_ipa(kvm, addr);
>  
>  			/* No need to invalidate the cache for device mappings */
> -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> +			if (((pte_val(old_pte) & PAGE_S2_DEVICE)
> +			     != PAGE_S2_DEVICE) &&
> +			    ((pte_val(old_pte) & PAGE_HYP_DEVICE)
> +			     != PAGE_HYP_DEVICE))
>  				kvm_flush_dcache_pte(old_pte);
>  
>  			put_page(virt_to_page(pte));
> -- 
> 2.4.4
> 

Did you check if PAGE_HYP_DEVICE can mean something sane on a stage-2
page table entry and vice verse?

Also, the commit message and formatting here is horrible, see this
reworked version:

>From e15700dd24419bb0e7ddc79feaa4efdf20304f2c Mon Sep 17 00:00:00 2001
From: Pavel Fedin <p.fedin@samsung.com>
Date: Tue, 27 Oct 2015 10:40:08 +0300
Subject: [PATCH] KVM: arm: Don't try to flush hyp-mode device mappings

The unmap_ptes function is currently called to unmap both Stage-2 and
Hyp mode page table entries.  Since calling clean and invalidate on
device memory may raise exceptions, we currently test against
PAGE_S2_DEVICE and do not flush for such mappings.  However, we should
also be testing against PAGE_HYP_DEVICE.

This fixes an issue observed on some 32-bit platforms, for example the
Exynos 5410.

Signed-off-by: Pavel Fedin <p.fedin@samsung.com>
Signed-off-by: Christoffer Dall <christoffer.dall@linaro.org>
---
 arch/arm/kvm/mmu.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
index 6984342..f0c3aef 100644
--- a/arch/arm/kvm/mmu.c
+++ b/arch/arm/kvm/mmu.c
@@ -206,18 +206,20 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
 
 	start_pte = pte = pte_offset_kernel(pmd, addr);
 	do {
-		if (!pte_none(*pte)) {
-			pte_t old_pte = *pte;
+		if (pte_none(*pte))
+			continue;
 
-			kvm_set_pte(pte, __pte(0));
-			kvm_tlb_flush_vmid_ipa(kvm, addr);
+		pte_t old_pte = *pte;
 
-			/* No need to invalidate the cache for device mappings */
-			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
-				kvm_flush_dcache_pte(old_pte);
+		kvm_set_pte(pte, __pte(0));
+		kvm_tlb_flush_vmid_ipa(kvm, addr);
 
-			put_page(virt_to_page(pte));
-		}
+		/* No need to invalidate the cache for device mappings */
+		if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE &&
+		    (pte_val(old_pte) & PAGE_HYP_DEVICE) != PAGE_HYP_DEVICE)
+			kvm_flush_dcache_pte(old_pte);
+
+		put_page(virt_to_page(pte));
 	} while (pte++, addr += PAGE_SIZE, addr != end);
 
 	if (kvm_pte_table_empty(kvm, start_pte))
-- 
2.1.2.330.g565301e.dirty

Re: [PATCH] KVM: arm: Don't try to flush hyp-mode device mappings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3238 bytes --]

Hi Christoffer,

[auto build test WARNING on: kvmarm/next]
[also build test WARNING on: v4.3 next-20151105]

url:    https://github.com/0day-ci/linux/commits/Christoffer-Dall/KVM-arm-Don-t-try-to-flush-hyp-mode-device-mappings/20151105-232548
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm64-defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All warnings (new ones prefixed by >>):

   arch/arm64/kvm/../../../arch/arm/kvm/mmu.c: In function 'unmap_ptes':
>> arch/arm64/kvm/../../../arch/arm/kvm/mmu.c:212:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
      pte_t old_pte = *pte;
      ^

vim +212 arch/arm64/kvm/../../../arch/arm/kvm/mmu.c

363ef89f8 Marc Zyngier     2014-12-19  196   *
363ef89f8 Marc Zyngier     2014-12-19  197   * This is why right after unmapping a page/section and invalidating
363ef89f8 Marc Zyngier     2014-12-19  198   * the corresponding TLBs, we call kvm_flush_dcache_p*() to make sure
363ef89f8 Marc Zyngier     2014-12-19  199   * the IO subsystem will never hit in the cache.
363ef89f8 Marc Zyngier     2014-12-19  200   */
4f853a714 Christoffer Dall 2014-05-09  201  static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
4f853a714 Christoffer Dall 2014-05-09  202  		       phys_addr_t addr, phys_addr_t end)
4f728276f Marc Zyngier     2013-04-12  203  {
4f853a714 Christoffer Dall 2014-05-09  204  	phys_addr_t start_addr = addr;
4f853a714 Christoffer Dall 2014-05-09  205  	pte_t *pte, *start_pte;
4f853a714 Christoffer Dall 2014-05-09  206  
4f853a714 Christoffer Dall 2014-05-09  207  	start_pte = pte = pte_offset_kernel(pmd, addr);
4f853a714 Christoffer Dall 2014-05-09  208  	do {
8e9a96138 Christoffer Dall 2015-11-05  209  		if (pte_none(*pte))
8e9a96138 Christoffer Dall 2015-11-05  210  			continue;
8e9a96138 Christoffer Dall 2015-11-05  211  
363ef89f8 Marc Zyngier     2014-12-19 @212  		pte_t old_pte = *pte;
363ef89f8 Marc Zyngier     2014-12-19  213  
4f728276f Marc Zyngier     2013-04-12  214  		kvm_set_pte(pte, __pte(0));
d4cb9df5d Marc Zyngier     2013-05-14  215  		kvm_tlb_flush_vmid_ipa(kvm, addr);
363ef89f8 Marc Zyngier     2014-12-19  216  
363ef89f8 Marc Zyngier     2014-12-19  217  		/* No need to invalidate the cache for device mappings */
8e9a96138 Christoffer Dall 2015-11-05  218  		if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE &&
8e9a96138 Christoffer Dall 2015-11-05  219  		    (pte_val(old_pte) & PAGE_HYP_DEVICE) != PAGE_HYP_DEVICE)
363ef89f8 Marc Zyngier     2014-12-19  220  			kvm_flush_dcache_pte(old_pte);

:::::: The code at line 212 was first introduced by commit
:::::: 363ef89f8e9bcedc28b976d0fe2d858fe139c122 arm/arm64: KVM: Invalidate data cache on unmap

:::::: TO: Marc Zyngier <marc.zyngier@arm.com>
:::::: CC: Christoffer Dall <christoffer.dall@linaro.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 16489 bytes --]

[-- Attachment #3: Type: text/plain, Size: 151 bytes --]

_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm

Re: [PATCH] KVM: arm: Don't try to flush hyp-mode device mappings

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3170 bytes --]

Hi Christoffer,

[auto build test WARNING on: kvmarm/next]
[also build test WARNING on: v4.3 next-20151105]

url:    https://github.com/0day-ci/linux/commits/Christoffer-Dall/KVM-arm-Don-t-try-to-flush-hyp-mode-device-mappings/20151105-232548
base:   https://git.kernel.org/pub/scm/linux/kernel/git/kvmarm/kvmarm.git next
config: arm-axm55xx_defconfig (attached as .config)
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All warnings (new ones prefixed by >>):

   arch/arm/kvm/mmu.c: In function 'unmap_ptes':
>> arch/arm/kvm/mmu.c:212:3: warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]
      pte_t old_pte = *pte;
      ^

vim +212 arch/arm/kvm/mmu.c

363ef89f8 Marc Zyngier     2014-12-19  196   *
363ef89f8 Marc Zyngier     2014-12-19  197   * This is why right after unmapping a page/section and invalidating
363ef89f8 Marc Zyngier     2014-12-19  198   * the corresponding TLBs, we call kvm_flush_dcache_p*() to make sure
363ef89f8 Marc Zyngier     2014-12-19  199   * the IO subsystem will never hit in the cache.
363ef89f8 Marc Zyngier     2014-12-19  200   */
4f853a714 Christoffer Dall 2014-05-09  201  static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
4f853a714 Christoffer Dall 2014-05-09  202  		       phys_addr_t addr, phys_addr_t end)
4f728276f Marc Zyngier     2013-04-12  203  {
4f853a714 Christoffer Dall 2014-05-09  204  	phys_addr_t start_addr = addr;
4f853a714 Christoffer Dall 2014-05-09  205  	pte_t *pte, *start_pte;
4f853a714 Christoffer Dall 2014-05-09  206  
4f853a714 Christoffer Dall 2014-05-09  207  	start_pte = pte = pte_offset_kernel(pmd, addr);
4f853a714 Christoffer Dall 2014-05-09  208  	do {
8e9a96138 Christoffer Dall 2015-11-05  209  		if (pte_none(*pte))
8e9a96138 Christoffer Dall 2015-11-05  210  			continue;
8e9a96138 Christoffer Dall 2015-11-05  211  
363ef89f8 Marc Zyngier     2014-12-19 @212  		pte_t old_pte = *pte;
363ef89f8 Marc Zyngier     2014-12-19  213  
4f728276f Marc Zyngier     2013-04-12  214  		kvm_set_pte(pte, __pte(0));
d4cb9df5d Marc Zyngier     2013-05-14  215  		kvm_tlb_flush_vmid_ipa(kvm, addr);
363ef89f8 Marc Zyngier     2014-12-19  216  
363ef89f8 Marc Zyngier     2014-12-19  217  		/* No need to invalidate the cache for device mappings */
8e9a96138 Christoffer Dall 2015-11-05  218  		if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE &&
8e9a96138 Christoffer Dall 2015-11-05  219  		    (pte_val(old_pte) & PAGE_HYP_DEVICE) != PAGE_HYP_DEVICE)
363ef89f8 Marc Zyngier     2014-12-19  220  			kvm_flush_dcache_pte(old_pte);

:::::: The code at line 212 was first introduced by commit
:::::: 363ef89f8e9bcedc28b976d0fe2d858fe139c122 arm/arm64: KVM: Invalidate data cache on unmap

:::::: TO: Marc Zyngier <marc.zyngier@arm.com>
:::::: CC: Christoffer Dall <christoffer.dall@linaro.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 18325 bytes --]

RE: [PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[Pavel Fedin]

 Hello!

> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 7b42012..839dd970 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -213,7 +213,10 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
> >  			kvm_tlb_flush_vmid_ipa(kvm, addr);
> >
> >  			/* No need to invalidate the cache for device mappings */
> > -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> > +			if (((pte_val(old_pte) & PAGE_S2_DEVICE)
> > +			     != PAGE_S2_DEVICE) &&
> > +			    ((pte_val(old_pte) & PAGE_HYP_DEVICE)
> > +			     != PAGE_HYP_DEVICE))
> >  				kvm_flush_dcache_pte(old_pte);
> >
> >  			put_page(virt_to_page(pte));
> > --
> > 2.4.4
> >
> 
> Did you check if PAGE_HYP_DEVICE can mean something sane on a stage-2
> page table entry and vice verse?

 I tried to, the chain of macros and variables is complicated enough not to
get 200% sure, but anyway PAGE_HYP_DEVICE (as well as PAGE_S2_DEVICE)
includes PROT_PTE_DEVICE, so this is definitely device.
 I even tried to construct some mask in order to make a single check for only
DEVICE flags, but, to make things even less understandable and predictable,
the same code with different bitfields is reused by ARM64. So, i thought that
it will be more reliable just to add a second test.

> 
> Also, the commit message and formatting here is horrible, see this
> reworked version:

[skip]

 It's OK, to tell the truth the commit message is not that much important
for me, but i know that sometimes these changes require good elaboration,
so i included as much information as possible, together with crash
backtrace. I've seen something like this in "git log" before.
 Could you give me some directions on how to write better messages? And
about formatting, IIRC i adhere to "75 chars per line" rule, and always
(well, almost, unless forget to do so ;) ) run checkpatch.

> diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> index 6984342..f0c3aef 100644
> --- a/arch/arm/kvm/mmu.c
> +++ b/arch/arm/kvm/mmu.c
> @@ -206,18 +206,20 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
> 
>  	start_pte = pte = pte_offset_kernel(pmd, addr);
>  	do {
> -		if (!pte_none(*pte)) {
> -			pte_t old_pte = *pte;
> +		if (pte_none(*pte))
> +			continue;
> 
> -			kvm_set_pte(pte, __pte(0));
> -			kvm_tlb_flush_vmid_ipa(kvm, addr);
> +		pte_t old_pte = *pte;
> 
> -			/* No need to invalidate the cache for device mappings */
> -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> -				kvm_flush_dcache_pte(old_pte);
> +		kvm_set_pte(pte, __pte(0));
> +		kvm_tlb_flush_vmid_ipa(kvm, addr);
> 
> -			put_page(virt_to_page(pte));
> -		}
> +		/* No need to invalidate the cache for device mappings */
> +		if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE &&
> +		    (pte_val(old_pte) & PAGE_HYP_DEVICE) != PAGE_HYP_DEVICE)
> +			kvm_flush_dcache_pte(old_pte);
> +
> +		put_page(virt_to_page(pte));
>  	} while (pte++, addr += PAGE_SIZE, addr != end);
> 
>  	if (kvm_pte_table_empty(kvm, start_pte))
> --

 I see you inverted pte_none() check, and now kbuild bot complains about
"mixed declarations and code".

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[Christoffer Dall]

On Fri, Nov 06, 2015 at 12:32:51PM +0300, Pavel Fedin wrote:
>  Hello!
> 
> > > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > > index 7b42012..839dd970 100644
> > > --- a/arch/arm/kvm/mmu.c
> > > +++ b/arch/arm/kvm/mmu.c
> > > @@ -213,7 +213,10 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
> > >  			kvm_tlb_flush_vmid_ipa(kvm, addr);
> > >
> > >  			/* No need to invalidate the cache for device mappings */
> > > -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> > > +			if (((pte_val(old_pte) & PAGE_S2_DEVICE)
> > > +			     != PAGE_S2_DEVICE) &&
> > > +			    ((pte_val(old_pte) & PAGE_HYP_DEVICE)
> > > +			     != PAGE_HYP_DEVICE))
> > >  				kvm_flush_dcache_pte(old_pte);
> > >
> > >  			put_page(virt_to_page(pte));
> > > --
> > > 2.4.4
> > >
> > 
> > Did you check if PAGE_HYP_DEVICE can mean something sane on a stage-2
> > page table entry and vice verse?
> 
>  I tried to, the chain of macros and variables is complicated enough not to
> get 200% sure, but anyway PAGE_HYP_DEVICE (as well as PAGE_S2_DEVICE)
> includes PROT_PTE_DEVICE, so this is definitely device.
>  I even tried to construct some mask in order to make a single check for only
> DEVICE flags, but, to make things even less understandable and predictable,
> the same code with different bitfields is reused by ARM64. So, i thought that
> it will be more reliable just to add a second test.

The thing I want to avoid is PAGE_HYP_DEVICE covering some normal S2
mapping, which we *should* flush but that we now end up ignoring?  That
doesn't sound like it can be the case because the device bit is the same
bit for both types of page tables, correct?

> 
> > 
> > Also, the commit message and formatting here is horrible, see this
> > reworked version:
> 
> [skip]
> 
>  It's OK, to tell the truth the commit message is not that much important
> for me, but i know that sometimes these changes require good elaboration,
> so i included as much information as possible, together with crash
> backtrace. I've seen something like this in "git log" before.

The problem with your commit message was that it focused on the symptom
of the bug and how you saw it, instead of describing what the commit
does and why the patch is correct from a general point of view.

Think about your commit subject if you do "git log --oneline", then with
your version I would think, "that commit addresses some weird bug that
happens only in certain circumstances" as opposed to "this commit
modifies the unmapping logic".

>  Could you give me some directions on how to write better messages? And
> about formatting, IIRC i adhere to "75 chars per line" rule, and always
> (well, almost, unless forget to do so ;) ) run checkpatch.

For mail text, I think the convention is to use 72 chars per line.

For code, it's advisable to stick to 80 chars per line, unless it makes
the code absolutely imposible to read (in your original patch, you
should just have let the lines be 82 chars or whatever it came to if you
wanted to do it that way).

> 
> > diff --git a/arch/arm/kvm/mmu.c b/arch/arm/kvm/mmu.c
> > index 6984342..f0c3aef 100644
> > --- a/arch/arm/kvm/mmu.c
> > +++ b/arch/arm/kvm/mmu.c
> > @@ -206,18 +206,20 @@ static void unmap_ptes(struct kvm *kvm, pmd_t *pmd,
> > 
> >  	start_pte = pte = pte_offset_kernel(pmd, addr);
> >  	do {
> > -		if (!pte_none(*pte)) {
> > -			pte_t old_pte = *pte;
> > +		if (pte_none(*pte))
> > +			continue;
> > 
> > -			kvm_set_pte(pte, __pte(0));
> > -			kvm_tlb_flush_vmid_ipa(kvm, addr);
> > +		pte_t old_pte = *pte;
> > 
> > -			/* No need to invalidate the cache for device mappings */
> > -			if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE)
> > -				kvm_flush_dcache_pte(old_pte);
> > +		kvm_set_pte(pte, __pte(0));
> > +		kvm_tlb_flush_vmid_ipa(kvm, addr);
> > 
> > -			put_page(virt_to_page(pte));
> > -		}
> > +		/* No need to invalidate the cache for device mappings */
> > +		if ((pte_val(old_pte) & PAGE_S2_DEVICE) != PAGE_S2_DEVICE &&
> > +		    (pte_val(old_pte) & PAGE_HYP_DEVICE) != PAGE_HYP_DEVICE)
> > +			kvm_flush_dcache_pte(old_pte);
> > +
> > +		put_page(virt_to_page(pte));
> >  	} while (pte++, addr += PAGE_SIZE, addr != end);
> > 
> >  	if (kvm_pte_table_empty(kvm, start_pte))
> > --
> 
>  I see you inverted pte_none() check, and now kbuild bot complains about
> "mixed declarations and code".
> 
Yeah, that can be easily fixed though.  I just did it quickly to
illustrate my point.  I'll fix that up if we can agree on the
PAGE_HYP_DEVICE and PAGE_S2_DEVICE stuff above.

-Christoffer

RE: [PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[Pavel Fedin]

 Hello!

> > > Did you check if PAGE_HYP_DEVICE can mean something sane on a stage-2
> > > page table entry and vice verse?
> >
> >  I tried to, the chain of macros and variables is complicated enough not to
> > get 200% sure, but anyway PAGE_HYP_DEVICE (as well as PAGE_S2_DEVICE)
> > includes PROT_PTE_DEVICE, so this is definitely device.
> >  I even tried to construct some mask in order to make a single check for only
> > DEVICE flags, but, to make things even less understandable and predictable,
> > the same code with different bitfields is reused by ARM64. So, i thought that
> > it will be more reliable just to add a second test.
> 
> The thing I want to avoid is PAGE_HYP_DEVICE covering some normal S2
> mapping, which we *should* flush but that we now end up ignoring?  That
> doesn't sound like it can be the case because the device bit is the same
> bit for both types of page tables, correct?

 Yes, this is exactly what i think. If DEVICE bit is set, then it's somehow
device memory and it doesn't need flashing.

 Or, in order to be 200% sure, we could modify the whole unmapping logic to carry
over a flag, telling whether we are removing normal or HYP mappings. But wouldn't
this be much more complicated?

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia

Re: [PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[Marc Zyngier]

On 06/11/15 13:43, Pavel Fedin wrote:
>  Hello!
> 
>>>> Did you check if PAGE_HYP_DEVICE can mean something sane on a stage-2
>>>> page table entry and vice verse?
>>>
>>>  I tried to, the chain of macros and variables is complicated enough not to
>>> get 200% sure, but anyway PAGE_HYP_DEVICE (as well as PAGE_S2_DEVICE)
>>> includes PROT_PTE_DEVICE, so this is definitely device.
>>>  I even tried to construct some mask in order to make a single check for only
>>> DEVICE flags, but, to make things even less understandable and predictable,
>>> the same code with different bitfields is reused by ARM64. So, i thought that
>>> it will be more reliable just to add a second test.
>>
>> The thing I want to avoid is PAGE_HYP_DEVICE covering some normal S2
>> mapping, which we *should* flush but that we now end up ignoring?  That
>> doesn't sound like it can be the case because the device bit is the same
>> bit for both types of page tables, correct?
> 
>  Yes, this is exactly what i think. If DEVICE bit is set, then it's somehow
> device memory and it doesn't need flashing.
> 
>  Or, in order to be 200% sure, we could modify the whole unmapping logic to carry
> over a flag, telling whether we are removing normal or HYP mappings. But wouldn't
> this be much more complicated?

We could do without that complexity. Also, the test itself is wrong (see
Ard's patch that was posted this morning for the real fix).

Thanks,

	M.
-- 
Jazz is not dead. It just smells funny...

RE: [PATCH] KVM: arm: Fix crash in free_hyp_pgds() if timer initialization fails

[Pavel Fedin]

 Hello!

> >> The thing I want to avoid is PAGE_HYP_DEVICE covering some normal S2
> >> mapping, which we *should* flush but that we now end up ignoring?  That
> >> doesn't sound like it can be the case because the device bit is the same
> >> bit for both types of page tables, correct?
> >
> >  Yes, this is exactly what i think. If DEVICE bit is set, then it's somehow
> > device memory and it doesn't need flashing.
> >
> >  Or, in order to be 200% sure, we could modify the whole unmapping logic to carry
> > over a flag, telling whether we are removing normal or HYP mappings. But wouldn't
> > this be much more complicated?
> 
> We could do without that complexity. Also, the test itself is wrong (see
> Ard's patch that was posted this morning for the real fix).

 Good. Saw it, will test it on monday. Indeed, this is better than my approach, and
this is what i actually wanted to do but didn't study the thing deeply enough to
implement.

Kind regards,
Pavel Fedin
Expert Engineer
Samsung Electronics Research center Russia