| Message ID | 20180517103548.5622-4-marc.zyngier@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, May 17, 2018 at 11:35:47AM +0100, Marc Zyngier wrote: > There is no need to perform cache maintenance operations when > creating the HYP page tables if we have the multiprocessing > extensions. ARMv7 mandates them with the virtualization support, > and ARMv8 just mandates them unconditionally. > > Let's remove these operations. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > virt/kvm/arm/mmu.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c > index ba66bf7ae299..acbfea09578c 100644 > --- a/virt/kvm/arm/mmu.c > +++ b/virt/kvm/arm/mmu.c > @@ -578,7 +578,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start, > pte = pte_offset_kernel(pmd, addr); > kvm_set_pte(pte, pfn_pte(pfn, prot)); > get_page(virt_to_page(pte)); > - kvm_flush_dcache_to_poc(pte, sizeof(*pte)); > pfn++; > } while (addr += PAGE_SIZE, addr != end); > } > @@ -605,7 +604,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start, > } > pmd_populate_kernel(NULL, pmd, pte); > get_page(virt_to_page(pmd)); > - kvm_flush_dcache_to_poc(pmd, sizeof(*pmd)); > } > > next = pmd_addr_end(addr, end); > @@ -638,7 +636,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start, > } > pud_populate(NULL, pud, pmd); > get_page(virt_to_page(pud)); > - kvm_flush_dcache_to_poc(pud, sizeof(*pud)); > } > > next = pud_addr_end(addr, end); > @@ -675,7 +672,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, > } > pgd_populate(NULL, pgd, pud); > get_page(virt_to_page(pgd)); > - kvm_flush_dcache_to_poc(pgd, sizeof(*pgd)); > } > > next = pgd_addr_end(addr, end); > @@ -685,6 +681,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, > pfn += (next - addr) >> PAGE_SHIFT; > } while (addr = next, addr != end); > out: > + dsb(ishst); > mutex_unlock(&kvm_hyp_pgd_mutex); Why do we need the DSB here? A comment would help. If a DMB is sufficient, I think mutex_unlock has release semantics.
On 24/05/18 16:51, Catalin Marinas wrote: > On Thu, May 17, 2018 at 11:35:47AM +0100, Marc Zyngier wrote: >> There is no need to perform cache maintenance operations when >> creating the HYP page tables if we have the multiprocessing >> extensions. ARMv7 mandates them with the virtualization support, >> and ARMv8 just mandates them unconditionally. >> >> Let's remove these operations. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> virt/kvm/arm/mmu.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c >> index ba66bf7ae299..acbfea09578c 100644 >> --- a/virt/kvm/arm/mmu.c >> +++ b/virt/kvm/arm/mmu.c >> @@ -578,7 +578,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start, >> pte = pte_offset_kernel(pmd, addr); >> kvm_set_pte(pte, pfn_pte(pfn, prot)); >> get_page(virt_to_page(pte)); >> - kvm_flush_dcache_to_poc(pte, sizeof(*pte)); >> pfn++; >> } while (addr += PAGE_SIZE, addr != end); >> } >> @@ -605,7 +604,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start, >> } >> pmd_populate_kernel(NULL, pmd, pte); >> get_page(virt_to_page(pmd)); >> - kvm_flush_dcache_to_poc(pmd, sizeof(*pmd)); >> } >> >> next = pmd_addr_end(addr, end); >> @@ -638,7 +636,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start, >> } >> pud_populate(NULL, pud, pmd); >> get_page(virt_to_page(pud)); >> - kvm_flush_dcache_to_poc(pud, sizeof(*pud)); >> } >> >> next = pud_addr_end(addr, end); >> @@ -675,7 +672,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, >> } >> pgd_populate(NULL, pgd, pud); >> get_page(virt_to_page(pgd)); >> - kvm_flush_dcache_to_poc(pgd, sizeof(*pgd)); >> } >> >> next = pgd_addr_end(addr, end); >> @@ -685,6 +681,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, >> pfn += (next - addr) >> PAGE_SHIFT; >> } while (addr = next, addr != end); >> out: >> + dsb(ishst); >> mutex_unlock(&kvm_hyp_pgd_mutex); > > Why do we need the DSB here? A comment would help. That's a combination of me being paranoid (we've just dropped a bunch of CMOs that had DSBs there), and prior art (see dcadda146f4f -- adding Mark who authored that change). My understanding is also that we do need this as per K11.5.3 (ARMv8 ARM rev C.a), which indicates that a DSB ISH "ensures visibility of the update to translation table walks". There is also this text: <quote> A translation table walk is considered to be a separate observer, and: - A write to the translation tables can be observed by that separate observer at any time after the execution of the instruction that performed that write, but is only guaranteed to be observable after the execution of a DSB instruction by the PE that executed the instruction that performed that write to the translation tables. </quote> I'm happy to add comment though. > If a DMB is sufficient, I think mutex_unlock has release semantics. I don't think it is, unfortunately. Thanks, M.
On Thu, May 17, 2018 at 11:35:47AM +0100, Marc Zyngier wrote: > There is no need to perform cache maintenance operations when > creating the HYP page tables if we have the multiprocessing > extensions. ARMv7 mandates them with the virtualization support, > and ARMv8 just mandates them unconditionally. > > Let's remove these operations. > > Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> > --- > virt/kvm/arm/mmu.c | 5 +---- > 1 file changed, 1 insertion(+), 4 deletions(-) > > diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c > index ba66bf7ae299..acbfea09578c 100644 > --- a/virt/kvm/arm/mmu.c > +++ b/virt/kvm/arm/mmu.c > @@ -578,7 +578,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start, > pte = pte_offset_kernel(pmd, addr); > kvm_set_pte(pte, pfn_pte(pfn, prot)); > get_page(virt_to_page(pte)); > - kvm_flush_dcache_to_poc(pte, sizeof(*pte)); > pfn++; > } while (addr += PAGE_SIZE, addr != end); > } > @@ -605,7 +604,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start, > } > pmd_populate_kernel(NULL, pmd, pte); > get_page(virt_to_page(pmd)); > - kvm_flush_dcache_to_poc(pmd, sizeof(*pmd)); > } > > next = pmd_addr_end(addr, end); > @@ -638,7 +636,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start, > } > pud_populate(NULL, pud, pmd); > get_page(virt_to_page(pud)); > - kvm_flush_dcache_to_poc(pud, sizeof(*pud)); > } > > next = pud_addr_end(addr, end); > @@ -675,7 +672,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, > } > pgd_populate(NULL, pgd, pud); > get_page(virt_to_page(pgd)); > - kvm_flush_dcache_to_poc(pgd, sizeof(*pgd)); > } > > next = pgd_addr_end(addr, end); > @@ -685,6 +681,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, > pfn += (next - addr) >> PAGE_SHIFT; > } while (addr = next, addr != end); > out: > + dsb(ishst); I think you need a dsb(ishst) wherever you had a kvm_flush_dcache_to_poc() previously. Otherwise, the page table walker could see stale values. e.g. after you update a bunch of PTE entries and point a PMD entry at those, the PTW could see the updated PMD entry, but see stale PTE entries, which could be garbage. That said, I think for ensuring the *order* those become visible in, you only need a dmb(ishst), and the ensure they *are visible* you need a DSB(ISHST) at the end. Thanks, Mark.
On 24/05/18 18:12, Mark Rutland wrote: > On Thu, May 17, 2018 at 11:35:47AM +0100, Marc Zyngier wrote: >> There is no need to perform cache maintenance operations when >> creating the HYP page tables if we have the multiprocessing >> extensions. ARMv7 mandates them with the virtualization support, >> and ARMv8 just mandates them unconditionally. >> >> Let's remove these operations. >> >> Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> >> --- >> virt/kvm/arm/mmu.c | 5 +---- >> 1 file changed, 1 insertion(+), 4 deletions(-) >> >> diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c >> index ba66bf7ae299..acbfea09578c 100644 >> --- a/virt/kvm/arm/mmu.c >> +++ b/virt/kvm/arm/mmu.c >> @@ -578,7 +578,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start, >> pte = pte_offset_kernel(pmd, addr); >> kvm_set_pte(pte, pfn_pte(pfn, prot)); >> get_page(virt_to_page(pte)); >> - kvm_flush_dcache_to_poc(pte, sizeof(*pte)); >> pfn++; >> } while (addr += PAGE_SIZE, addr != end); >> } >> @@ -605,7 +604,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start, >> } >> pmd_populate_kernel(NULL, pmd, pte); >> get_page(virt_to_page(pmd)); >> - kvm_flush_dcache_to_poc(pmd, sizeof(*pmd)); >> } >> >> next = pmd_addr_end(addr, end); >> @@ -638,7 +636,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start, >> } >> pud_populate(NULL, pud, pmd); >> get_page(virt_to_page(pud)); >> - kvm_flush_dcache_to_poc(pud, sizeof(*pud)); >> } >> >> next = pud_addr_end(addr, end); >> @@ -675,7 +672,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, >> } >> pgd_populate(NULL, pgd, pud); >> get_page(virt_to_page(pgd)); >> - kvm_flush_dcache_to_poc(pgd, sizeof(*pgd)); >> } >> >> next = pgd_addr_end(addr, end); >> @@ -685,6 +681,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, >> pfn += (next - addr) >> PAGE_SHIFT; >> } while (addr = next, addr != end); >> out: >> + dsb(ishst); > > I think you need a dsb(ishst) wherever you had a > kvm_flush_dcache_to_poc() previously. > > Otherwise, the page table walker could see stale values. e.g. after you > update a bunch of PTE entries and point a PMD entry at those, the PTW > could see the updated PMD entry, but see stale PTE entries, which could > be garbage. That's a very good point. I initially only considered the EL2 initialisation (the EL2 MMU is not live yet), but failed to consider the additional mappings at runtime (each time we instantiate a VM). > That said, I think for ensuring the *order* those become visible in, you > only need a dmb(ishst), and the ensure they *are visible* you need a > DSB(ISHST) at the end. And we definitely want the latter. I'll respin this shortly. Thanks, M.
diff --git a/virt/kvm/arm/mmu.c b/virt/kvm/arm/mmu.c index ba66bf7ae299..acbfea09578c 100644 --- a/virt/kvm/arm/mmu.c +++ b/virt/kvm/arm/mmu.c @@ -578,7 +578,6 @@ static void create_hyp_pte_mappings(pmd_t *pmd, unsigned long start, pte = pte_offset_kernel(pmd, addr); kvm_set_pte(pte, pfn_pte(pfn, prot)); get_page(virt_to_page(pte)); - kvm_flush_dcache_to_poc(pte, sizeof(*pte)); pfn++; } while (addr += PAGE_SIZE, addr != end); } @@ -605,7 +604,6 @@ static int create_hyp_pmd_mappings(pud_t *pud, unsigned long start, } pmd_populate_kernel(NULL, pmd, pte); get_page(virt_to_page(pmd)); - kvm_flush_dcache_to_poc(pmd, sizeof(*pmd)); } next = pmd_addr_end(addr, end); @@ -638,7 +636,6 @@ static int create_hyp_pud_mappings(pgd_t *pgd, unsigned long start, } pud_populate(NULL, pud, pmd); get_page(virt_to_page(pud)); - kvm_flush_dcache_to_poc(pud, sizeof(*pud)); } next = pud_addr_end(addr, end); @@ -675,7 +672,6 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, } pgd_populate(NULL, pgd, pud); get_page(virt_to_page(pgd)); - kvm_flush_dcache_to_poc(pgd, sizeof(*pgd)); } next = pgd_addr_end(addr, end); @@ -685,6 +681,7 @@ static int __create_hyp_mappings(pgd_t *pgdp, unsigned long ptrs_per_pgd, pfn += (next - addr) >> PAGE_SHIFT; } while (addr = next, addr != end); out: + dsb(ishst); mutex_unlock(&kvm_hyp_pgd_mutex); return err; }
There is no need to perform cache maintenance operations when creating the HYP page tables if we have the multiprocessing extensions. ARMv7 mandates them with the virtualization support, and ARMv8 just mandates them unconditionally. Let's remove these operations. Signed-off-by: Marc Zyngier <marc.zyngier@arm.com> --- virt/kvm/arm/mmu.c | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-)