* [kvm-unit-tests PATCH v2 0/2] arm/arm64: teach virt_to_pte_phys() about block descriptors
@ 2022-11-24 15:28 ` Alexandru Elisei
0 siblings, 0 replies; 7+ messages in thread
From: Alexandru Elisei @ 2022-11-24 15:28 UTC (permalink / raw)
To: andrew.jones, kvm, kvmarm, kvmarm
I was writing a quick test when I noticed that arm's implementation of
__virt_to_phys(), which ends up calling virt_to_pte_phys(), doesn't handle
block mappings and returns a bogus value. When fixing it I got confused
about mmu_get_pte() and get_pte(), so I (hopefully) improved things by
renaming mmu_get_pte() to follow_pte().
Changes since v1:
- Dropped patch #1 ("lib/vmalloc: Treat virt_to_pte_phys() as returning a
physical address") because it was incorrect.
- Dropped the check for pte_valid() for the return value of mmu_get_pte() in
patch #1 because mmu_get_pte() returns NULL for an invalid descriptor.
Lightly tested on a rockpro64 (4k and 64k pages, arm64 and arm, qemu only)
because the changes from the previous version are trivial.
Alexandru Elisei (2):
arm/arm64: mmu: Teach virt_to_pte_phys() about block descriptors
arm/arm64: mmu: Rename mmu_get_pte() -> follow_pte()
lib/arm/asm/mmu-api.h | 2 +-
lib/arm/mmu.c | 88 +++++++++++++++++++++++++------------------
2 files changed, 53 insertions(+), 37 deletions(-)
--
2.37.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply [flat|nested] 7+ messages in thread* [kvm-unit-tests PATCH v2 1/2] arm/arm64: mmu: Teach virt_to_pte_phys() about block descriptors
2022-11-24 15:28 ` Alexandru Elisei
@ 2022-11-24 15:28 ` Alexandru Elisei
-1 siblings, 0 replies; 7+ messages in thread
From: Alexandru Elisei @ 2022-11-24 15:28 UTC (permalink / raw)
To: andrew.jones, kvm, kvmarm, kvmarm
The arm and arm64 architectures allow a virtual address to be mapped using
a block descriptor (or huge page, as Linux calls it), and the function
mmu_set_ranges_sect() is made available for a test to do just that. But
virt_to_pte_phys() assumes that all virtual addresses are mapped with page
granularity, which can lead to erroneous addresses being returned in the
case of block mappings.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/mmu.c | 89 +++++++++++++++++++++++++++++++--------------------
1 file changed, 54 insertions(+), 35 deletions(-)
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index e1a72fe4941f..6022e356ddd4 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -111,10 +111,61 @@ pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *virt)
__pgprot(PTE_WBWA | PTE_USER));
}
-phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *mem)
+/*
+ * NOTE: The Arm architecture might require the use of a
+ * break-before-make sequence before making changes to a PTE and
+ * certain conditions are met (see Arm ARM D5-2669 for AArch64 and
+ * B3-1378 for AArch32 for more details).
+ */
+pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
{
- return (*get_pte(pgtable, (uintptr_t)mem) & PHYS_MASK & -PAGE_SIZE)
- + ((ulong)mem & (PAGE_SIZE - 1));
+ pgd_t *pgd;
+ pud_t *pud;
+ pmd_t *pmd;
+ pte_t *pte;
+
+ if (!mmu_enabled())
+ return NULL;
+
+ pgd = pgd_offset(pgtable, vaddr);
+ if (!pgd_valid(*pgd))
+ return NULL;
+
+ pud = pud_offset(pgd, vaddr);
+ if (!pud_valid(*pud))
+ return NULL;
+
+ pmd = pmd_offset(pud, vaddr);
+ if (!pmd_valid(*pmd))
+ return NULL;
+ if (pmd_huge(*pmd))
+ return &pmd_val(*pmd);
+
+ pte = pte_offset(pmd, vaddr);
+ if (!pte_valid(*pte))
+ return NULL;
+
+ return &pte_val(*pte);
+}
+
+phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *virt)
+{
+ phys_addr_t mask;
+ pteval_t *pteval;
+
+ pteval = mmu_get_pte(pgtable, (uintptr_t)virt);
+ if (!pteval) {
+ install_page(pgtable, (phys_addr_t)(unsigned long)virt, virt);
+ return (phys_addr_t)(unsigned long)virt;
+ }
+
+ if (pmd_huge(__pmd(*pteval)))
+ mask = PMD_MASK;
+ else
+ mask = PAGE_MASK;
+
+ return (*pteval & PHYS_MASK & mask) |
+ ((phys_addr_t)(unsigned long)virt & ~mask);
}
void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
@@ -231,38 +282,6 @@ unsigned long __phys_to_virt(phys_addr_t addr)
return addr;
}
-/*
- * NOTE: The Arm architecture might require the use of a
- * break-before-make sequence before making changes to a PTE and
- * certain conditions are met (see Arm ARM D5-2669 for AArch64 and
- * B3-1378 for AArch32 for more details).
- */
-pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
-{
- pgd_t *pgd;
- pud_t *pud;
- pmd_t *pmd;
- pte_t *pte;
-
- if (!mmu_enabled())
- return NULL;
-
- pgd = pgd_offset(pgtable, vaddr);
- assert(pgd_valid(*pgd));
- pud = pud_offset(pgd, vaddr);
- assert(pud_valid(*pud));
- pmd = pmd_offset(pud, vaddr);
- assert(pmd_valid(*pmd));
-
- if (pmd_huge(*pmd))
- return &pmd_val(*pmd);
-
- pte = pte_offset(pmd, vaddr);
- assert(pte_valid(*pte));
-
- return &pte_val(*pte);
-}
-
void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
{
pteval_t *p_pte = mmu_get_pte(pgtable, vaddr);
--
2.37.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [kvm-unit-tests PATCH v2 1/2] arm/arm64: mmu: Teach virt_to_pte_phys() about block descriptors
@ 2022-11-24 15:28 ` Alexandru Elisei
0 siblings, 0 replies; 7+ messages in thread
From: Alexandru Elisei @ 2022-11-24 15:28 UTC (permalink / raw)
To: andrew.jones, kvm, kvmarm, kvmarm
The arm and arm64 architectures allow a virtual address to be mapped using
a block descriptor (or huge page, as Linux calls it), and the function
mmu_set_ranges_sect() is made available for a test to do just that. But
virt_to_pte_phys() assumes that all virtual addresses are mapped with page
granularity, which can lead to erroneous addresses being returned in the
case of block mappings.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/mmu.c | 89 +++++++++++++++++++++++++++++++--------------------
1 file changed, 54 insertions(+), 35 deletions(-)
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index e1a72fe4941f..6022e356ddd4 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -111,10 +111,61 @@ pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *virt)
__pgprot(PTE_WBWA | PTE_USER));
}
-phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *mem)
+/*
+ * NOTE: The Arm architecture might require the use of a
+ * break-before-make sequence before making changes to a PTE and
+ * certain conditions are met (see Arm ARM D5-2669 for AArch64 and
+ * B3-1378 for AArch32 for more details).
+ */
+pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
{
- return (*get_pte(pgtable, (uintptr_t)mem) & PHYS_MASK & -PAGE_SIZE)
- + ((ulong)mem & (PAGE_SIZE - 1));
+ pgd_t *pgd;
+ pud_t *pud;
+ pmd_t *pmd;
+ pte_t *pte;
+
+ if (!mmu_enabled())
+ return NULL;
+
+ pgd = pgd_offset(pgtable, vaddr);
+ if (!pgd_valid(*pgd))
+ return NULL;
+
+ pud = pud_offset(pgd, vaddr);
+ if (!pud_valid(*pud))
+ return NULL;
+
+ pmd = pmd_offset(pud, vaddr);
+ if (!pmd_valid(*pmd))
+ return NULL;
+ if (pmd_huge(*pmd))
+ return &pmd_val(*pmd);
+
+ pte = pte_offset(pmd, vaddr);
+ if (!pte_valid(*pte))
+ return NULL;
+
+ return &pte_val(*pte);
+}
+
+phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *virt)
+{
+ phys_addr_t mask;
+ pteval_t *pteval;
+
+ pteval = mmu_get_pte(pgtable, (uintptr_t)virt);
+ if (!pteval) {
+ install_page(pgtable, (phys_addr_t)(unsigned long)virt, virt);
+ return (phys_addr_t)(unsigned long)virt;
+ }
+
+ if (pmd_huge(__pmd(*pteval)))
+ mask = PMD_MASK;
+ else
+ mask = PAGE_MASK;
+
+ return (*pteval & PHYS_MASK & mask) |
+ ((phys_addr_t)(unsigned long)virt & ~mask);
}
void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
@@ -231,38 +282,6 @@ unsigned long __phys_to_virt(phys_addr_t addr)
return addr;
}
-/*
- * NOTE: The Arm architecture might require the use of a
- * break-before-make sequence before making changes to a PTE and
- * certain conditions are met (see Arm ARM D5-2669 for AArch64 and
- * B3-1378 for AArch32 for more details).
- */
-pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
-{
- pgd_t *pgd;
- pud_t *pud;
- pmd_t *pmd;
- pte_t *pte;
-
- if (!mmu_enabled())
- return NULL;
-
- pgd = pgd_offset(pgtable, vaddr);
- assert(pgd_valid(*pgd));
- pud = pud_offset(pgd, vaddr);
- assert(pud_valid(*pud));
- pmd = pmd_offset(pud, vaddr);
- assert(pmd_valid(*pmd));
-
- if (pmd_huge(*pmd))
- return &pmd_val(*pmd);
-
- pte = pte_offset(pmd, vaddr);
- assert(pte_valid(*pte));
-
- return &pte_val(*pte);
-}
-
void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
{
pteval_t *p_pte = mmu_get_pte(pgtable, vaddr);
--
2.37.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [kvm-unit-tests PATCH v2 2/2] arm/arm64: mmu: Rename mmu_get_pte() -> follow_pte()
2022-11-24 15:28 ` Alexandru Elisei
@ 2022-11-24 15:28 ` Alexandru Elisei
-1 siblings, 0 replies; 7+ messages in thread
From: Alexandru Elisei @ 2022-11-24 15:28 UTC (permalink / raw)
To: andrew.jones, kvm, kvmarm, kvmarm
The function get_pte() from mmu.c returns a pointer to the PTE
associated with the requested virtual address, mapping the virtual
address in the process if it's not already mapped.
mmu_get_pte() returns a pointer to the PTE if and only if the virtual is
mapped in pgtable, otherwise returns NULL. Rename it to follow_pte() to
avoid any confusion with get_pte(). follow_pte() also matches the name
of Linux kernel function with a similar purpose.
Also remove the mmu_enabled() check from the function, as the purpose of
the function is to get the mapping for the virtual address in the pgtable
supplied as the argument, not to translate the virtual address to a
physical address using the current translation; that's what
virt_to_phys() does.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/mmu-api.h | 2 +-
lib/arm/mmu.c | 9 +++------
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
index 3d77cbfd8b24..6c1136d957f9 100644
--- a/lib/arm/asm/mmu-api.h
+++ b/lib/arm/asm/mmu-api.h
@@ -17,6 +17,6 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
phys_addr_t phys_start, phys_addr_t phys_end,
pgprot_t prot);
-extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr);
+extern pteval_t *follow_pte(pgd_t *pgtable, uintptr_t vaddr);
extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
#endif
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 6022e356ddd4..18e32b2b8927 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -117,16 +117,13 @@ pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *virt)
* certain conditions are met (see Arm ARM D5-2669 for AArch64 and
* B3-1378 for AArch32 for more details).
*/
-pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
+pteval_t *follow_pte(pgd_t *pgtable, uintptr_t vaddr)
{
pgd_t *pgd;
pud_t *pud;
pmd_t *pmd;
pte_t *pte;
- if (!mmu_enabled())
- return NULL;
-
pgd = pgd_offset(pgtable, vaddr);
if (!pgd_valid(*pgd))
return NULL;
@@ -153,7 +150,7 @@ phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *virt)
phys_addr_t mask;
pteval_t *pteval;
- pteval = mmu_get_pte(pgtable, (uintptr_t)virt);
+ pteval = follow_pte(pgtable, (uintptr_t)virt);
if (!pteval) {
install_page(pgtable, (phys_addr_t)(unsigned long)virt, virt);
return (phys_addr_t)(unsigned long)virt;
@@ -284,7 +281,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
{
- pteval_t *p_pte = mmu_get_pte(pgtable, vaddr);
+ pteval_t *p_pte = follow_pte(pgtable, vaddr);
if (p_pte) {
pteval_t entry = *p_pte & ~PTE_USER;
WRITE_ONCE(*p_pte, entry);
--
2.37.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [kvm-unit-tests PATCH v2 2/2] arm/arm64: mmu: Rename mmu_get_pte() -> follow_pte()
@ 2022-11-24 15:28 ` Alexandru Elisei
0 siblings, 0 replies; 7+ messages in thread
From: Alexandru Elisei @ 2022-11-24 15:28 UTC (permalink / raw)
To: andrew.jones, kvm, kvmarm, kvmarm
The function get_pte() from mmu.c returns a pointer to the PTE
associated with the requested virtual address, mapping the virtual
address in the process if it's not already mapped.
mmu_get_pte() returns a pointer to the PTE if and only if the virtual is
mapped in pgtable, otherwise returns NULL. Rename it to follow_pte() to
avoid any confusion with get_pte(). follow_pte() also matches the name
of Linux kernel function with a similar purpose.
Also remove the mmu_enabled() check from the function, as the purpose of
the function is to get the mapping for the virtual address in the pgtable
supplied as the argument, not to translate the virtual address to a
physical address using the current translation; that's what
virt_to_phys() does.
Signed-off-by: Alexandru Elisei <alexandru.elisei@arm.com>
---
lib/arm/asm/mmu-api.h | 2 +-
lib/arm/mmu.c | 9 +++------
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/lib/arm/asm/mmu-api.h b/lib/arm/asm/mmu-api.h
index 3d77cbfd8b24..6c1136d957f9 100644
--- a/lib/arm/asm/mmu-api.h
+++ b/lib/arm/asm/mmu-api.h
@@ -17,6 +17,6 @@ extern void mmu_set_range_sect(pgd_t *pgtable, uintptr_t virt_offset,
extern void mmu_set_range_ptes(pgd_t *pgtable, uintptr_t virt_offset,
phys_addr_t phys_start, phys_addr_t phys_end,
pgprot_t prot);
-extern pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr);
+extern pteval_t *follow_pte(pgd_t *pgtable, uintptr_t vaddr);
extern void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr);
#endif
diff --git a/lib/arm/mmu.c b/lib/arm/mmu.c
index 6022e356ddd4..18e32b2b8927 100644
--- a/lib/arm/mmu.c
+++ b/lib/arm/mmu.c
@@ -117,16 +117,13 @@ pteval_t *install_page(pgd_t *pgtable, phys_addr_t phys, void *virt)
* certain conditions are met (see Arm ARM D5-2669 for AArch64 and
* B3-1378 for AArch32 for more details).
*/
-pteval_t *mmu_get_pte(pgd_t *pgtable, uintptr_t vaddr)
+pteval_t *follow_pte(pgd_t *pgtable, uintptr_t vaddr)
{
pgd_t *pgd;
pud_t *pud;
pmd_t *pmd;
pte_t *pte;
- if (!mmu_enabled())
- return NULL;
-
pgd = pgd_offset(pgtable, vaddr);
if (!pgd_valid(*pgd))
return NULL;
@@ -153,7 +150,7 @@ phys_addr_t virt_to_pte_phys(pgd_t *pgtable, void *virt)
phys_addr_t mask;
pteval_t *pteval;
- pteval = mmu_get_pte(pgtable, (uintptr_t)virt);
+ pteval = follow_pte(pgtable, (uintptr_t)virt);
if (!pteval) {
install_page(pgtable, (phys_addr_t)(unsigned long)virt, virt);
return (phys_addr_t)(unsigned long)virt;
@@ -284,7 +281,7 @@ unsigned long __phys_to_virt(phys_addr_t addr)
void mmu_clear_user(pgd_t *pgtable, unsigned long vaddr)
{
- pteval_t *p_pte = mmu_get_pte(pgtable, vaddr);
+ pteval_t *p_pte = follow_pte(pgtable, vaddr);
if (p_pte) {
pteval_t entry = *p_pte & ~PTE_USER;
WRITE_ONCE(*p_pte, entry);
--
2.37.0
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [kvm-unit-tests PATCH v2 0/2] arm/arm64: teach virt_to_pte_phys() about block descriptors
2022-11-24 15:28 ` Alexandru Elisei
` (2 preceding siblings ...)
(?)
@ 2023-02-14 20:23 ` Andrew Jones
-1 siblings, 0 replies; 7+ messages in thread
From: Andrew Jones @ 2023-02-14 20:23 UTC (permalink / raw)
To: Alexandru Elisei; +Cc: kvm, kvmarm, kvmarm
On Thu, Nov 24, 2022 at 03:28:14PM +0000, Alexandru Elisei wrote:
> I was writing a quick test when I noticed that arm's implementation of
> __virt_to_phys(), which ends up calling virt_to_pte_phys(), doesn't handle
> block mappings and returns a bogus value. When fixing it I got confused
> about mmu_get_pte() and get_pte(), so I (hopefully) improved things by
> renaming mmu_get_pte() to follow_pte().
>
> Changes since v1:
>
> - Dropped patch #1 ("lib/vmalloc: Treat virt_to_pte_phys() as returning a
> physical address") because it was incorrect.
> - Dropped the check for pte_valid() for the return value of mmu_get_pte() in
> patch #1 because mmu_get_pte() returns NULL for an invalid descriptor.
>
> Lightly tested on a rockpro64 (4k and 64k pages, arm64 and arm, qemu only)
> because the changes from the previous version are trivial.
>
> Alexandru Elisei (2):
> arm/arm64: mmu: Teach virt_to_pte_phys() about block descriptors
> arm/arm64: mmu: Rename mmu_get_pte() -> follow_pte()
>
> lib/arm/asm/mmu-api.h | 2 +-
> lib/arm/mmu.c | 88 +++++++++++++++++++++++++------------------
> 2 files changed, 53 insertions(+), 37 deletions(-)
>
> --
> 2.37.0
>
Applied, thanks
^ permalink raw reply [flat|nested] 7+ messages in thread