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
