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

2015-11-06 Thread 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


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-06 Thread 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


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-06 Thread 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


--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-06 Thread 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...
--
To unsubscribe from this list: send the line "unsubscribe kvm" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2015-11-06 Thread 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
--
To unsubscribe from this list:

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

2015-11-05 Thread 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=8040006003, *pmd=
> Internal error: Oops: 2a06 [#1] PREEMPT SMP ARM
> ...
> [] (v7_flush_kern_dcache_area) from [] 
> (kvm_flush_dcache_pte+0x48/0x5c)
> [] (kvm_flush_dcache_pte) from [] 
> (unmap_range+0x24c/0x460)
> [] (unmap_range) from [] (free_hyp_pgds+0x84/0x160)
> [] (free_hyp_pgds) from [] (kvm_arch_init+0x254/0x41c)
> [] (kvm_arch_init) from [] (kvm_init+0x28/0x2b4)
> [] (kvm_init) from [] (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 
> ---
>  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 
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 
Signed-off-by: Christoffer Dall 
---
 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);
+
+